From 1b9c8377ae6d9c61fd4a488f3c3d822cf37e47bd Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 5 Nov 2024 23:23:06 +0100 Subject: [PATCH] Hash existing API keys, and do checks against the hash --- .../Core/migrations/Version20241105215309.php | 45 +++++++++++++++++++ module/Rest/src/Entity/ApiKey.php | 3 +- module/Rest/src/Service/ApiKeyService.php | 3 +- .../Repository/ApiKeyRepositoryTest.php | 4 +- .../Rest/test/Service/ApiKeyServiceTest.php | 16 +++++-- 5 files changed, 61 insertions(+), 10 deletions(-) create mode 100644 module/Core/migrations/Version20241105215309.php diff --git a/module/Core/migrations/Version20241105215309.php b/module/Core/migrations/Version20241105215309.php new file mode 100644 index 00000000..0e9f7eff --- /dev/null +++ b/module/Core/migrations/Version20241105215309.php @@ -0,0 +1,45 @@ +connection->quoteIdentifier('key'); + + $qb = $this->connection->createQueryBuilder(); + $qb->select($keyColumnName) + ->from('api_keys'); + $result = $qb->executeQuery(); + + $updateQb = $this->connection->createQueryBuilder(); + $updateQb + ->update('api_keys') + ->set($keyColumnName, ':encryptedKey') + ->where($updateQb->expr()->eq($keyColumnName, ':plainTextKey')); + + while ($key = $result->fetchOne()) { + $updateQb->setParameters([ + 'encryptedKey' => hash('sha256', $key), + 'plainTextKey' => $key, + ])->executeStatement(); + } + } + + public function isTransactional(): bool + { + return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform); + } +} diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index 8a72b85e..32f8fff4 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -44,8 +44,7 @@ class ApiKey extends AbstractEntity */ 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); } diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php index 9b731a55..ca43a81f 100644 --- a/module/Rest/src/Service/ApiKeyService.php +++ b/module/Rest/src/Service/ApiKeyService.php @@ -67,8 +67,7 @@ readonly class ApiKeyService implements ApiKeyServiceInterface private function getByKey(string $key): ApiKey|null { return $this->em->getRepository(ApiKey::class)->findOneBy([ -// 'key' => ApiKey::hashKey($key), - 'key' => $key, + 'key' => ApiKey::hashKey($key), ]); } } diff --git a/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php b/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php index c19d8512..62d52de6 100644 --- a/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php +++ b/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php @@ -24,9 +24,9 @@ class ApiKeyRepositoryTest extends DatabaseTestCase 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')])); } } diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index d6f49fb6..b081b99a 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/module/Rest/test/Service/ApiKeyServiceTest.php @@ -74,7 +74,9 @@ class ApiKeyServiceTest extends TestCase #[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'); @@ -97,7 +99,9 @@ class ApiKeyServiceTest extends TestCase { $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'); @@ -109,7 +113,9 @@ class ApiKeyServiceTest extends TestCase #[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); @@ -121,7 +127,9 @@ class ApiKeyServiceTest extends TestCase 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');