Skip to content

Commit

Permalink
Hash existing API keys, and do checks against the hash
Browse files Browse the repository at this point in the history
  • Loading branch information
acelaya committed Nov 5, 2024
1 parent 9f69751 commit e6b1a38
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 10 deletions.
45 changes: 45 additions & 0 deletions module/Core/migrations/Version20241105215309.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

declare(strict_types=1);

namespace ShlinkMigrations;

use Doctrine\DBAL\Platforms\MySQLPlatform;
use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

use function hash;

/**
* Hash API keys as SHA256
*/
final class Version20241105215309 extends AbstractMigration
{
public function up(Schema $schema): void
{
$keyIdentifier = $this->connection->quoteIdentifier('key');

$qb = $this->connection->createQueryBuilder();
$qb->select($keyIdentifier)
->from('api_keys');
$result = $qb->executeQuery();

$updateQb = $this->connection->createQueryBuilder();
$updateQb
->update('api_keys')
->set($keyIdentifier, ':encryptedKey')
->where($updateQb->expr()->eq($keyIdentifier, ':plainTextKey'));

while ($key = $result->fetchOne()) {
$updateQb->setParameters([
'encryptedKey' => hash('sha256', $key),
'plainTextKey' => $key,
])->executeStatement();
}
}

public function isTransactional(): bool
{
return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform);
}
}
3 changes: 1 addition & 2 deletions module/Rest/src/Entity/ApiKey.php
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ public static function create(): ApiKey
*/
public static function fromMeta(ApiKeyMeta $meta): self
{
// $apiKey = new self(self::hashKey($meta->key), $meta->name, $meta->expirationDate);
$apiKey = new self($meta->key, $meta->name, $meta->expirationDate);
$apiKey = new self(self::hashKey($meta->key), $meta->name, $meta->expirationDate);
foreach ($meta->roleDefinitions as $roleDefinition) {
$apiKey->registerRole($roleDefinition);
}
Expand Down
3 changes: 1 addition & 2 deletions module/Rest/src/Service/ApiKeyService.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,7 @@ public function listKeys(bool $enabledOnly = false): array
private function getByKey(string $key): ApiKey|null
{
return $this->em->getRepository(ApiKey::class)->findOneBy([
// 'key' => ApiKey::hashKey($key),
'key' => $key,
'key' => ApiKey::hashKey($key),
]);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ public function initialApiKeyIsCreatedOnlyOfNoApiKeysExistYet(): void
self::assertCount(0, $this->repo->findAll());
self::assertNotNull($this->repo->createInitialApiKey('initial_value'));
self::assertCount(1, $this->repo->findAll());
self::assertCount(1, $this->repo->findBy(['key' => 'initial_value']));
self::assertCount(1, $this->repo->findBy(['key' => ApiKey::hashKey('initial_value')]));
self::assertNull($this->repo->createInitialApiKey('another_one'));
self::assertCount(1, $this->repo->findAll());
self::assertCount(0, $this->repo->findBy(['key' => 'another_one']));
self::assertCount(0, $this->repo->findBy(['key' => ApiKey::hashKey('another_one')]));
}
}
16 changes: 12 additions & 4 deletions module/Rest/test/Service/ApiKeyServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ public static function provideCreationDate(): iterable
#[Test, DataProvider('provideInvalidApiKeys')]
public function checkReturnsFalseForInvalidApiKeys(ApiKey|null $invalidKey): void
{
$this->repo->expects($this->once())->method('findOneBy')->with(['key' => '12345'])->willReturn($invalidKey);
$this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn(
$invalidKey,
);
$this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo);

$result = $this->service->check('12345');
Expand All @@ -97,7 +99,9 @@ public function checkReturnsTrueWhenConditionsAreFavorable(): void
{
$apiKey = ApiKey::create();

$this->repo->expects($this->once())->method('findOneBy')->with(['key' => '12345'])->willReturn($apiKey);
$this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn(
$apiKey,
);
$this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo);

$result = $this->service->check('12345');
Expand All @@ -109,7 +113,9 @@ public function checkReturnsTrueWhenConditionsAreFavorable(): void
#[Test]
public function disableThrowsExceptionWhenNoApiKeyIsFound(): void
{
$this->repo->expects($this->once())->method('findOneBy')->with(['key' => '12345'])->willReturn(null);
$this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn(
null,
);
$this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo);

$this->expectException(InvalidArgumentException::class);
Expand All @@ -121,7 +127,9 @@ public function disableThrowsExceptionWhenNoApiKeyIsFound(): void
public function disableReturnsDisabledApiKeyWhenFound(): void
{
$key = ApiKey::create();
$this->repo->expects($this->once())->method('findOneBy')->with(['key' => '12345'])->willReturn($key);
$this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn(
$key,
);
$this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo);
$this->em->expects($this->once())->method('flush');

Expand Down

0 comments on commit e6b1a38

Please sign in to comment.