Hash existing API keys, and do checks against the hash

shlinkio · Nov 5, 2024 · 1b9c837 · 1b9c837
1 parent 9f69751
commit 1b9c837
Show file tree

Hide file tree

Showing 5 changed files with 61 additions and 10 deletions.
diff --git a/module/Core/migrations/Version20241105215309.php b/module/Core/migrations/Version20241105215309.php
@@ -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
+    {
+        $keyColumnName = $this->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
@@ -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);
         }

diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php
@@ -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),
         ]);
     }
 }
diff --git a/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php b/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php
@@ -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')]));
     }
 }
diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php
@@ -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');
@@ -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');
@@ -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);
@@ -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');