diff --git a/module/CLI/config/cli.config.php b/module/CLI/config/cli.config.php index a554db40..669a3788 100644 --- a/module/CLI/config/cli.config.php +++ b/module/CLI/config/cli.config.php @@ -26,6 +26,7 @@ return [ Command\Api\GenerateKeyCommand::NAME => Command\Api\GenerateKeyCommand::class, Command\Api\DisableKeyCommand::NAME => Command\Api\DisableKeyCommand::class, + Command\Api\DeleteKeyCommand::NAME => Command\Api\DeleteKeyCommand::class, Command\Api\ListKeysCommand::NAME => Command\Api\ListKeysCommand::class, Command\Api\InitialApiKeyCommand::NAME => Command\Api\InitialApiKeyCommand::class, Command\Api\RenameApiKeyCommand::NAME => Command\Api\RenameApiKeyCommand::class, diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index 5df74822..b4bf15d2 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -52,6 +52,7 @@ return [ Command\Api\GenerateKeyCommand::class => ConfigAbstractFactory::class, Command\Api\DisableKeyCommand::class => ConfigAbstractFactory::class, + Command\Api\DeleteKeyCommand::class => ConfigAbstractFactory::class, Command\Api\ListKeysCommand::class => ConfigAbstractFactory::class, Command\Api\InitialApiKeyCommand::class => ConfigAbstractFactory::class, Command\Api\RenameApiKeyCommand::class => ConfigAbstractFactory::class, @@ -108,6 +109,7 @@ return [ Command\Api\GenerateKeyCommand::class => [ApiKeyService::class, ApiKey\RoleResolver::class], Command\Api\DisableKeyCommand::class => [ApiKeyService::class], + Command\Api\DeleteKeyCommand::class => [ApiKeyService::class], Command\Api\ListKeysCommand::class => [ApiKeyService::class], Command\Api\InitialApiKeyCommand::class => [ApiKeyService::class], Command\Api\RenameApiKeyCommand::class => [ApiKeyService::class], diff --git a/module/CLI/src/Command/Api/DeleteKeyCommand.php b/module/CLI/src/Command/Api/DeleteKeyCommand.php new file mode 100644 index 00000000..f57a5e4a --- /dev/null +++ b/module/CLI/src/Command/Api/DeleteKeyCommand.php @@ -0,0 +1,94 @@ +%command.name% command allows you to delete an existing API key via its name. + + If no arguments are provided, you will be prompted to select one of the existing API keys. + + %command.full_name% + + You can optionally pass the API key name to be disabled: + + %command.full_name% the_key_name + + HELP, +)] +class DeleteKeyCommand extends Command +{ + public const string NAME = 'api-key:delete'; + + public function __construct(private readonly ApiKeyServiceInterface $apiKeyService) + { + parent::__construct(); + } + + protected function interact(InputInterface $input, OutputInterface $output): void + { + $apiKeyName = $input->getArgument('name'); + + if ($apiKeyName === null) { + $apiKeys = $this->apiKeyService->listKeys(); + $name = (new SymfonyStyle($input, $output))->choice( + 'What API key do you want to delete?', + map($apiKeys, static fn (ApiKey $apiKey) => $apiKey->name), + ); + + $input->setArgument('name', $name); + } + } + + public function __invoke( + SymfonyStyle $io, + InputInterface $input, + #[Argument(description: 'The API key to delete.')] + string|null $name = null, + ): int { + if ($name === null) { + $io->warning('An API key name was not provided.'); + return Command::INVALID; + } + + if (! $this->shouldProceed($io, $input)) { + return Command::INVALID; + } + + try { + $this->apiKeyService->deleteByName($name); + $io->success(sprintf('API key "%s" properly deleted', $name)); + return Command::SUCCESS; + } catch (ApiKeyNotFoundException $e) { + $io->error($e->getMessage()); + return Command::FAILURE; + } + } + + private function shouldProceed(SymfonyStyle $io, InputInterface $input): bool + { + if (! $input->isInteractive()) { + return true; + } + + $io->warning('You are about to delete an API key. This action cannot be undone.'); + return $io->confirm('Are you sure you want to delete the API key?'); + } +} diff --git a/module/CLI/test/Command/Api/DeleteKeyCommandTest.php b/module/CLI/test/Command/Api/DeleteKeyCommandTest.php new file mode 100644 index 00000000..db61423d --- /dev/null +++ b/module/CLI/test/Command/Api/DeleteKeyCommandTest.php @@ -0,0 +1,100 @@ +apiKeyService = $this->createMock(ApiKeyServiceInterface::class); + $this->commandTester = CliTestUtils::testerForCommand(new DeleteKeyCommand($this->apiKeyService)); + } + + #[Test] + public function warningIsReturnedIfNoArgumentIsProvidedInNonInteractiveMode(): void + { + $this->apiKeyService->expects($this->never())->method('deleteByName'); + $this->apiKeyService->expects($this->never())->method('listKeys'); + + $exitCode = $this->commandTester->execute([], ['interactive' => false]); + + self::assertEquals(Command::INVALID, $exitCode); + } + + #[Test] + public function confirmationIsSkippedInNonInteractiveMode(): void + { + $this->apiKeyService->expects($this->once())->method('deleteByName'); + $this->apiKeyService->expects($this->never())->method('listKeys'); + + $exitCode = $this->commandTester->execute(['name' => 'key to delete'], ['interactive' => false]); + $output = $this->commandTester->getDisplay(); + + self::assertEquals(Command::SUCCESS, $exitCode); + self::assertStringNotContainsString('Are you sure you want to delete the API key?', $output); + } + + #[Test] + public function keyIsNotDeletedIfConfirmationIsCancelled(): void + { + $this->apiKeyService->expects($this->never())->method('deleteByName'); + $this->apiKeyService->expects($this->never())->method('listKeys'); + + $this->commandTester->setInputs(['no']); + $exitCode = $this->commandTester->execute(['name' => 'key_to_delete']); + + self::assertEquals(Command::INVALID, $exitCode); + } + + #[Test] + public function existingApiKeyNamesAreListedIfNoArgumentIsProvidedInInteractiveMode(): void + { + $name = 'the key to delete'; + $this->apiKeyService->expects($this->once())->method('deleteByName')->with($name); + $this->apiKeyService->expects($this->once())->method('listKeys')->willReturn([ + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'foo')), + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: $name)), + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'bar')), + ]); + + $this->commandTester->setInputs([$name, 'y']); + $exitCode = $this->commandTester->execute([]); + $output = $this->commandTester->getDisplay(); + + self::assertStringContainsString('What API key do you want to delete?', $output); + self::assertStringContainsString('API key "the key to delete" properly deleted', $output); + self::assertEquals(Command::SUCCESS, $exitCode); + } + + #[Test] + public function errorIsReturnedIfDisableByKeyThrowsException(): void + { + $apiKey = 'key to delete'; + $e = ApiKeyNotFoundException::forName($apiKey); + $this->apiKeyService->expects($this->once())->method('deleteByName')->with($apiKey)->willThrowException($e); + $this->apiKeyService->expects($this->never())->method('listKeys'); + + $exitCode = $this->commandTester->execute(['name' => $apiKey]); + $output = $this->commandTester->getDisplay(); + + self::assertStringContainsString($e->getMessage(), $output); + self::assertEquals(Command::FAILURE, $exitCode); + } +} diff --git a/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php b/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php index 6b282a07..8a6bc59d 100644 --- a/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php +++ b/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\ApiKey\Repository; use Doctrine\DBAL\LockMode; +use Doctrine\ORM\QueryBuilder; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -48,10 +49,9 @@ class ApiKeyRepository extends EntitySpecificationRepository implements ApiKeyRe { $qb = $this->getEntityManager()->createQueryBuilder(); $qb->select('a.id') - ->from(ApiKey::class, 'a') - ->where($qb->expr()->eq('a.name', ':name')) - ->setParameter('name', $name) - ->setMaxResults(1); + ->from(ApiKey::class, 'a'); + + $this->queryBuilderByName($qb, $name); // Lock for update, to avoid a race condition that inserts a duplicate name after we have checked if one existed $query = $qb->getQuery(); @@ -59,4 +59,27 @@ class ApiKeyRepository extends EntitySpecificationRepository implements ApiKeyRe return $query->getOneOrNullResult() !== null; } + + /** + * @inheritDoc + */ + public function deleteByName(string $name): int + { + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->delete(ApiKey::class, 'a'); + + $this->queryBuilderByName($qb, $name); + + return $qb->getQuery()->execute(); + } + + /** + * Apply a condition by name to a query builder, and ensure only one result is returned + */ + private function queryBuilderByName(QueryBuilder $qb, string $name): void + { + $qb->where($qb->expr()->eq('a.name', ':name')) + ->setParameter('name', $name) + ->setMaxResults(1); + } } diff --git a/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php b/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php index 32ada38a..3c4d5397 100644 --- a/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php +++ b/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php @@ -22,4 +22,10 @@ interface ApiKeyRepositoryInterface extends EntityRepositoryInterface, EntitySpe * Checks whether an API key with provided name exists or not */ public function nameExists(string $name): bool; + + /** + * Delete an API key by name + * @return positive-int|0 Number of affected results + */ + public function deleteByName(string $name): int; } diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php index 5779e86c..a5a85b79 100644 --- a/module/Rest/src/Service/ApiKeyService.php +++ b/module/Rest/src/Service/ApiKeyService.php @@ -68,6 +68,17 @@ readonly class ApiKeyService implements ApiKeyServiceInterface return new ApiKeyCheckResult($apiKey); } + /** + * @inheritDoc + */ + public function deleteByName(string $apiKeyName): void + { + $affectedResults = $this->repo->deleteByName($apiKeyName); + if ($affectedResults === 0) { + throw ApiKeyNotFoundException::forName($apiKeyName); + } + } + /** * @inheritDoc */ diff --git a/module/Rest/src/Service/ApiKeyServiceInterface.php b/module/Rest/src/Service/ApiKeyServiceInterface.php index 745355d7..6eb3df0f 100644 --- a/module/Rest/src/Service/ApiKeyServiceInterface.php +++ b/module/Rest/src/Service/ApiKeyServiceInterface.php @@ -22,6 +22,11 @@ interface ApiKeyServiceInterface public function check(string $key): ApiKeyCheckResult; + /** + * @throws ApiKeyNotFoundException + */ + public function deleteByName(string $apiKeyName): void; + /** * @throws ApiKeyNotFoundException */ diff --git a/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php b/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php index d0f6157d..6b3423e9 100644 --- a/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php +++ b/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php @@ -40,4 +40,18 @@ class ApiKeyRepositoryTest extends DatabaseTestCase self::assertTrue($this->repo->nameExists('foo')); self::assertFalse($this->repo->nameExists('bar')); } + + #[Test] + public function deleteByNameReturnsExpectedValue(): void + { + $this->getEntityManager()->persist(ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'foo'))); + $this->getEntityManager()->flush(); + $this->getEntityManager()->clear(); + + self::assertEquals(0, $this->repo->deleteByName('invalid')); + self::assertEquals(1, $this->repo->deleteByName('foo')); + + // Verify the API key has been deleted + self::assertNull($this->repo->findOneBy(['name' => 'foo'])); + } } diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index a5b6cecb..146ce0ac 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/module/Rest/test/Service/ApiKeyServiceTest.php @@ -8,6 +8,7 @@ use Cake\Chronos\Chronos; use Doctrine\ORM\EntityManager; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\TestWith; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; @@ -268,4 +269,19 @@ class ApiKeyServiceTest extends TestCase self::assertSame($apiKey, $result); self::assertEquals('new', $apiKey->name); } + + #[Test] + #[TestWith([0, true])] + #[TestWith([1, false])] + public function deleteByNameThrowsIfNoResultsAreAffected(int $affectedResults, bool $shouldThrow): void + { + $name = 'some_name'; + $this->repo->expects($this->once())->method('deleteByName')->with($name)->willReturn($affectedResults); + + if ($shouldThrow) { + $this->expectException(ApiKeyNotFoundException::class); + } + + $this->service->deleteByName($name); + } }