shlinkio/shlink
1
0
Fork 0
mirror of https://github.com/shlinkio/shlink.git synced 2025-12-10 11:05:50 -06:00
Code Issues Packages Projects Releases 146 Wiki Activity

Merge pull request #2523 from acelaya-forks/no-disable-by-api-key

Browse Source 
Do not allow API keys to be disabled by plain-text key
This commit is contained in:
Alejandro Celaya 2025-11-08 09:34:34 +01:00 committed by GitHub
commit d2bc9f7c2b
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 26 additions and 113 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
* [#2507](https://github.com/shlinkio/shlink/issues/2507) Drop support for PHP 8.3.
* [#2514](https://github.com/shlinkio/shlink/issues/2514) Remove support to generate QR codes. This functionality is now handled by Shlink Web Client and Shlink Dashboard.
* [#2517](https://github.com/shlinkio/shlink/issues/2517) Remove `REDIRECT_APPEND_EXTRA_PATH` env var. Use `REDIRECT_EXTRA_PATH_MODE=append` instead.
* [#2519](https://github.com/shlinkio/shlink/issues/2519) Disabling API keys by their plain-text key is no longer supported. When calling `api-key:disable`, the first argument is now always assumed to be the name.
### Fixed
* *Nothing*

42
module/CLI/src/Command/Api/DisableKeyCommand.php
View File

@ -9,7 +9,6 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey;
use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface;
use Symfony\Component\Console\Attribute\Argument;
use Symfony\Component\Console\Attribute\AsCommand;
use Symfony\Component\Console\Attribute\Option;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
@ -20,24 +19,17 @@ use function sprintf;
#[AsCommand(
name: DisableKeyCommand::NAME,
description: 'Disables an API key by name or plain-text key (providing a plain-text key is DEPRECATED)',
description: 'Disables an API key by name',
help: <<<HELP
The <info>%command.name%</info> command allows you to disable an existing API key, via its name or the
plain-text key.
The <info>%command.name%</info> command allows you to disable an existing API key.
If no arguments are provided, you will be prompted to select one of the existing non-disabled API keys.
<info>%command.full_name%</info>
You can optionally pass the API key name to be disabled. In that case <comment>--by-name</comment> is also
required, to indicate the first argument is the API key name and not the plain-text key:
You can optionally pass the API key name to be disabled:
<info>%command.full_name% the_key_name --by-name</info>
You can pass the plain-text key to be disabled, but that is <options=bold>DEPRECATED</>. In next major version,
the argument will always be assumed to be the name:
<info>%command.full_name% d6b6c60e-edcd-4e43-96ad-fa6b7014c143</info>
<info>%command.full_name% the_key_name</info>
HELP,
)]
@ -52,41 +44,31 @@ class DisableKeyCommand extends Command
protected function interact(InputInterface $input, OutputInterface $output): void
{
$keyOrName = $input->getArgument('key-or-name');
$name = $input->getArgument('name');
if ($keyOrName === null) {
if ($name === null) {
$apiKeys = $this->apiKeyService->listKeys(enabledOnly: true);
$name = (new SymfonyStyle($input, $output))->choice(
$name = new SymfonyStyle($input, $output)->choice(
'What API key do you want to disable?',
map($apiKeys, static fn (ApiKey $apiKey) => $apiKey->name),
);
$input->setArgument('key-or-name', $name);
$input->setOption('by-name', true);
$input->setArgument('name', $name);
}
}
public function __invoke(
SymfonyStyle $io,
#[Argument(
description: 'The API key to disable. Pass `--by-name` to indicate this value is the name and not the key.',
)]
string|null $keyOrName = null,
#[Option(description: 'Indicates the first argument is the API key name, not the plain-text key.')]
bool $byName = false,
#[Argument('The name of the API key to disable.')] string|null $name = null,
): int {
if ($keyOrName === null) {
if ($name === null) {
$io->warning('An API key name was not provided.');
return Command::INVALID;
}
try {
if ($byName) {
$this->apiKeyService->disableByName($keyOrName);
} else {
$this->apiKeyService->disableByKey($keyOrName);
}
$io->success(sprintf('API key "%s" properly disabled', $keyOrName));
$this->apiKeyService->disableByName($name);
$io->success(sprintf('API key "%s" properly disabled', $name));
return Command::SUCCESS;
} catch (InvalidArgumentException $e) {
$io->error($e->getMessage());

55
module/CLI/test/Command/Api/DisableKeyCommandTest.php
View File

@ -31,12 +31,9 @@ class DisableKeyCommandTest extends TestCase
public function providedApiKeyIsDisabled(): void
{
$apiKey = 'abcd1234';
$this->apiKeyService->expects($this->once())->method('disableByKey')->with($apiKey);
$this->apiKeyService->expects($this->never())->method('disableByName');
$this->apiKeyService->expects($this->once())->method('disableByName')->with($apiKey);
$exitCode = $this->commandTester->execute([
'key-or-name' => $apiKey,
]);
$exitCode = $this->commandTester->execute(['name' => $apiKey]);
$output = $this->commandTester->getDisplay();
self::assertStringContainsString('API key "abcd1234" properly disabled', $output);
@ -44,55 +41,15 @@ class DisableKeyCommandTest extends TestCase
}
#[Test]
public function providedApiKeyIsDisabledByName(): void
{
$name = 'the key to delete';
$this->apiKeyService->expects($this->once())->method('disableByName')->with($name);
$this->apiKeyService->expects($this->never())->method('disableByKey');
$exitCode = $this->commandTester->execute([
'key-or-name' => $name,
'--by-name' => true,
]);
$output = $this->commandTester->getDisplay();
self::assertStringContainsString('API key "the key to delete" properly disabled', $output);
self::assertEquals(Command::SUCCESS, $exitCode);
}
#[Test]
public function errorIsReturnedIfDisableByKeyThrowsException(): void
public function errorIsReturnedIfDisableByNameThrowsException(): void
{
$apiKey = 'abcd1234';
$expectedMessage = 'API key "abcd1234" does not exist.';
$this->apiKeyService->expects($this->once())->method('disableByKey')->with($apiKey)->willThrowException(
$this->apiKeyService->expects($this->once())->method('disableByName')->with($apiKey)->willThrowException(
new InvalidArgumentException($expectedMessage),
);
$this->apiKeyService->expects($this->never())->method('disableByName');
$exitCode = $this->commandTester->execute([
'key-or-name' => $apiKey,
]);
$output = $this->commandTester->getDisplay();
self::assertStringContainsString($expectedMessage, $output);
self::assertEquals(Command::FAILURE, $exitCode);
}
#[Test]
public function errorIsReturnedIfDisableByNameThrowsException(): void
{
$name = 'the key to delete';
$expectedMessage = 'API key "the key to delete" does not exist.';
$this->apiKeyService->expects($this->once())->method('disableByName')->with($name)->willThrowException(
new InvalidArgumentException($expectedMessage),
);
$this->apiKeyService->expects($this->never())->method('disableByKey');
$exitCode = $this->commandTester->execute([
'key-or-name' => $name,
'--by-name' => true,
]);
$exitCode = $this->commandTester->execute(['name' => $apiKey]);
$output = $this->commandTester->getDisplay();
self::assertStringContainsString($expectedMessage, $output);
@ -103,7 +60,6 @@ class DisableKeyCommandTest extends TestCase
public function warningIsReturnedIfNoArgumentIsProvidedInNonInteractiveMode(): void
{
$this->apiKeyService->expects($this->never())->method('disableByName');
$this->apiKeyService->expects($this->never())->method('disableByKey');
$this->apiKeyService->expects($this->never())->method('listKeys');
$exitCode = $this->commandTester->execute([], ['interactive' => false]);
@ -121,7 +77,6 @@ class DisableKeyCommandTest extends TestCase
ApiKey::fromMeta(ApiKeyMeta::fromParams(name: $name)),
ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'bar')),
]);
$this->apiKeyService->expects($this->never())->method('disableByKey');
$this->commandTester->setInputs([$name]);
$exitCode = $this->commandTester->execute([]);

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

@ -92,19 +92,6 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
return $this->disableApiKey($apiKey);
}
/**
* @inheritDoc
*/
public function disableByKey(string $key): ApiKey
{
$apiKey = $this->findByKey($key);
if ($apiKey === null) {
throw ApiKeyNotFoundException::forKey($key);
}
return $this->disableApiKey($apiKey);
}
private function disableApiKey(ApiKey $apiKey): ApiKey
{
$apiKey->disable();

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

@ -32,12 +32,6 @@ interface ApiKeyServiceInterface
*/
public function disableByName(string $apiKeyName): ApiKey;
/**
* @deprecated Use `self::disableByName($name)` instead
* @throws ApiKeyNotFoundException
*/
public function disableByKey(string $key): ApiKey;
/**
* @return ApiKey[]
*/

22
module/Rest/test/Service/ApiKeyServiceTest.php
View File

@ -143,35 +143,29 @@ class ApiKeyServiceTest extends TestCase
self::assertSame($apiKey, $result->apiKey);
}
#[Test, DataProvider('provideDisableArgs')]
public function disableThrowsExceptionWhenNoApiKeyIsFound(string $disableMethod, array $findOneByArg): void
#[Test]
public function disableThrowsExceptionWhenNoApiKeyIsFound(): void
{
$this->repo->expects($this->once())->method('findOneBy')->with($findOneByArg)->willReturn(null);
$this->repo->expects($this->once())->method('findOneBy')->with(['name' => '12345'])->willReturn(null);
$this->expectException(ApiKeyNotFoundException::class);
$this->service->{$disableMethod}('12345');
$this->service->disableByName('12345');
}
#[Test, DataProvider('provideDisableArgs')]
public function disableReturnsDisabledApiKeyWhenFound(string $disableMethod, array $findOneByArg): void
#[Test]
public function disableReturnsDisabledApiKeyWhenFound(): void
{
$key = ApiKey::create();
$this->repo->expects($this->once())->method('findOneBy')->with($findOneByArg)->willReturn($key);
$this->repo->expects($this->once())->method('findOneBy')->with(['name' => '12345'])->willReturn($key);
$this->em->expects($this->once())->method('flush');
self::assertTrue($key->isEnabled());
$returnedKey = $this->service->{$disableMethod}('12345');
$returnedKey = $this->service->disableByName('12345');
self::assertFalse($key->isEnabled());
self::assertSame($key, $returnedKey);
}
public static function provideDisableArgs(): iterable
{
yield 'disableByKey' => ['disableByKey', ['key' => ApiKey::hashKey('12345')]];
yield 'disableByName' => ['disableByName', ['name' => '12345']];
}
#[Test]
public function listFindsAllApiKeys(): void
{