Move logic to determine if a new key has a duplicated name to the APiKeyService

This commit is contained in:
Alejandro Celaya 2024-11-08 09:03:50 +01:00
parent b08c498b13
commit 6f837b3b91
6 changed files with 38 additions and 50 deletions

View File

@ -9,9 +9,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this
* [#2207](https://github.com/shlinkio/shlink/issues/2207) Add `hasRedirectRules` flag to short URL API model. This flag tells if a specific short URL has any redirect rules attached to it.
* [#1520](https://github.com/shlinkio/shlink/issues/1520) Allow short URLs list to be filtered by `domain`.
This change applies both to the `GET /short-urls` endpoint, via the `domain` query parameter, and the `short-url:list` console command, via the `--domain`|`-d` flag.
This change applies both to the `GET /short-urls` endpoint, via the `domain` query parameter, and the `short-url:list` console command, via the `--domain`|`-d` flag.
### Changed
* [#2193](https://github.com/shlinkio/shlink/issues/2193) API keys are now hashed using SHA256, instead of being saved in plain text.
As a side effect, API key names have now become more important, and are considered unique.
When people update to this Shlink version, existing API keys will be hashed for everything to continue working.
In order to avoid data to be lost, plain-text keys will be written in the `name` field, either together with any existing name, or as the name itself. Then users are responsible for renaming them using the new `api-key:rename` command.
For newly created API keys, it is recommended to provide a name, but if not provided, a name will be generated from a redacted version of the new API key.
* Update to Shlink PHP coding standard 2.4
* Update to `hidehalo/nanoid-php` 2.0

View File

@ -108,13 +108,6 @@ class GenerateKeyCommand extends Command
roleDefinitions: $this->roleResolver->determineRoles($input),
);
if ($this->apiKeyService->existsWithName($apiKeyMeta->name)) {
$io->warning(
sprintf('An API key with name "%s" already exists. Try with a different ome', $apiKeyMeta->name),
);
return ExitCode::EXIT_WARNING;
}
$apiKey = $this->apiKeyService->create($apiKeyMeta);
$io->success(sprintf('Generated API key: "%s"', $apiKeyMeta->key));

View File

@ -71,21 +71,4 @@ class GenerateKeyCommandTest extends TestCase
self::assertEquals(ExitCode::EXIT_SUCCESS, $exitCode);
}
#[Test]
public function warningIsPrintedIfProvidedNameAlreadyExists(): void
{
$name = 'The API key';
$this->apiKeyService->expects($this->never())->method('create');
$this->apiKeyService->expects($this->once())->method('existsWithName')->with($name)->willReturn(true);
$exitCode = $this->commandTester->execute([
'--name' => $name,
]);
$output = $this->commandTester->getDisplay();
self::assertEquals(ExitCode::EXIT_WARNING, $exitCode);
self::assertStringContainsString('An API key with name "The API key" already exists.', $output);
}
}

View File

@ -21,7 +21,13 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
public function create(ApiKeyMeta $apiKeyMeta): ApiKey
{
// TODO If name is auto-generated, do not throw. Instead, re-generate a new key
$apiKey = ApiKey::fromMeta($apiKeyMeta);
if ($this->existsWithName($apiKey->name)) {
throw new InvalidArgumentException(
sprintf('Another API key with name "%s" already exists', $apiKeyMeta->name),
);
}
$this->em->persist($apiKey);
$this->em->flush();
@ -77,14 +83,6 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
return $this->repo->findBy($conditions);
}
/**
* @inheritDoc
*/
public function existsWithName(string $apiKeyName): bool
{
return $this->repo->count(['name' => $apiKeyName]) > 0;
}
/**
* @inheritDoc
* @todo This method should be transactional and to a SELECT ... FROM UPDATE when checking if the new name exists,
@ -120,4 +118,9 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
{
return $this->repo->findOneBy(['key' => ApiKey::hashKey($key)]);
}
private function existsWithName(string $apiKeyName): bool
{
return $this->repo->count(['name' => $apiKeyName]) > 0;
}
}

View File

@ -33,11 +33,6 @@ interface ApiKeyServiceInterface
*/
public function listKeys(bool $enabledOnly = false): array;
/**
* 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
*/

View File

@ -8,7 +8,6 @@ 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;
@ -41,6 +40,9 @@ class ApiKeyServiceTest extends TestCase
#[Test, DataProvider('provideCreationDate')]
public function apiKeyIsProperlyCreated(Chronos|null $date, string|null $name, array $roles): void
{
$this->repo->expects($this->once())->method('count')->with(
! empty($name) ? ['name' => $name] : $this->isType('array'),
)->willReturn(0);
$this->em->expects($this->once())->method('flush');
$this->em->expects($this->once())->method('persist')->with($this->isInstanceOf(ApiKey::class));
@ -73,6 +75,19 @@ class ApiKeyServiceTest extends TestCase
yield 'empty name' => [null, '', []];
}
#[Test]
public function exceptionIsThrownWhileCreatingIfNameIsInUse(): void
{
$this->repo->expects($this->once())->method('count')->with(['name' => 'the_name'])->willReturn(1);
$this->em->expects($this->never())->method('flush');
$this->em->expects($this->never())->method('persist');
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('Another API key with name "the_name" already exists');
$this->service->create(ApiKeyMeta::fromParams(name: 'the_name'));
}
#[Test, DataProvider('provideInvalidApiKeys')]
public function checkReturnsFalseForInvalidApiKeys(ApiKey|null $invalidKey): void
{
@ -179,17 +194,6 @@ class ApiKeyServiceTest extends TestCase
yield 'existing api keys' => [null];
}
#[Test]
#[TestWith([0, false])]
#[TestWith([1, true])]
#[TestWith([27, true])]
public function existsWithNameCountsEntriesInRepository(int $count, bool $expected): void
{
$name = 'the_key';
$this->repo->expects($this->once())->method('count')->with(['name' => $name])->willReturn($count);
self::assertEquals($this->service->existsWithName($name), $expected);
}
#[Test]
public function renameApiKeyThrowsExceptionIfApiKeyIsNotFound(): void
{