diff --git a/README.md b/README.md index 3d2fc89..6b13c2f 100644 --- a/README.md +++ b/README.md @@ -191,6 +191,20 @@ It works with every `AUTH_METHOD` and never contacts the backend: a wrong secret The secret is shown once at creation and stored hashed. Keep it on the server side, never in a browser, and revoke the key (dashboard, or `davis:app-key:revoke `) when the service goes away; an expiry date can be set when the access is meant to be temporary. This is the use case of [#183](https://github.com/tchapi/davis/issues/183). +Revoked keys stay listed. To remove one for good, use `davis:app-key:delete ` (an active key must be revoked first). To clean up in bulk, `davis:app-key:prune ` deletes, in one go, every key revoked (or expired) at least `` days ago; `--dry-run` shows what would go. A key with an expiry date still ahead is active and is never deleted by either command. Deleting a key also deletes its usage history, so pick `` according to how long you want to keep it. + +Davis does not schedule the prune itself. If you want it, add a cron job, for example every night at 3am with a 90 days retention: + +```shell +0 3 * * * cd /path/to/davis && bin/console davis:app-key:prune 90 -q +``` + +or, with the docker image: + +```shell +0 3 * * * docker exec davis sh -c "APP_ENV=prod bin/console davis:app-key:prune 90 -q" +``` + **Update check** ```shell diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 2654d1f..9cb8fdd 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -209,12 +209,6 @@ - - - getAppId()]]> - getAppId()]]> - - diff --git a/src/Command/AppKeyDeleteCommand.php b/src/Command/AppKeyDeleteCommand.php new file mode 100644 index 0000000..524f622 --- /dev/null +++ b/src/Command/AppKeyDeleteCommand.php @@ -0,0 +1,65 @@ +appKeys->isEnabled(); + } + + protected function configure(): void + { + $this + ->setDescription('Delete a revoked (or expired) app key') + ->addArgument('app-id', InputArgument::REQUIRED, 'The app id of the key, as shown by davis:app-key:list') + ->setHelp('This command deletes a revoked (or expired) app key for good, with its usage history. An active key (including one with an expiry date still ahead) must be revoked first.'); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + $appId = $input->getArgument('app-id'); + + $key = $this->repository->findOneByAppId($appId); + + if (null === $key) { + $io->error(sprintf('No app key with the app id %s.', $appId)); + + return self::FAILURE; + } + + if ($key->isActive(new \DateTimeImmutable())) { + $io->error(sprintf('%s is still active. Revoke it first with davis:app-key:revoke.', $appId)); + + return self::FAILURE; + } + + $this->repository->delete([$key]); + + $io->success(sprintf('Deleted %s.', $appId)); + + return self::SUCCESS; + } +} diff --git a/src/Command/AppKeyPruneCommand.php b/src/Command/AppKeyPruneCommand.php new file mode 100644 index 0000000..13dd787 --- /dev/null +++ b/src/Command/AppKeyPruneCommand.php @@ -0,0 +1,100 @@ +appKeys->isEnabled(); + } + + protected function configure(): void + { + $this + ->setDescription('Delete the app keys revoked (or expired) a while ago') + ->addArgument('days', InputArgument::REQUIRED, 'Delete the keys revoked at least this many days ago (0 for every revoked key, '.self::MAX_DAYS.' at most)') + ->addOption('dry-run', null, InputOption::VALUE_NONE, 'List the keys that would be deleted, delete nothing') + ->setHelp(<<<'HELP' + This command deletes the app keys that were revoked (or that expired) at least the given number + of days ago, with their usage history (which users each key acted as, and when). Choose the + number of days according to how long you want to keep that history for. All the keys are + deleted in one transaction: either they all go, or none does. + + A key whose expiry date is still ahead is active and is never touched. Keys that were never + revoked are not touched either, use davis:app-key:revoke first. + + Davis does not run this on its own. To do it every night, for example: + + 0 3 * * * cd /path/to/davis && bin/console davis:app-key:prune 90 -q + + Use --dry-run to see what would be deleted. + HELP); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + $days = $input->getArgument('days'); + + // Bounded: PHP throws on a DateInterval with a huge number of days, and (int) would saturate + $days = filter_var($days, FILTER_VALIDATE_INT, ['options' => ['min_range' => 0, 'max_range' => self::MAX_DAYS]]); + if (false === $days) { + $io->error(sprintf('"%s" is not a number of days, expected an integer between 0 and %d.', $input->getArgument('days'), self::MAX_DAYS)); + + return self::INVALID; + } + + $dryRun = $input->getOption('dry-run'); + $cutoff = (new \DateTimeImmutable())->sub(new \DateInterval(sprintf('P%dD', $days))); + + $keys = $this->repository->findRevokedBefore($cutoff); + + if (!$keys) { + $io->note(sprintf('No app key revoked before %s, nothing to delete.', $cutoff->format('Y-m-d H:i'))); + + return self::SUCCESS; + } + + foreach ($keys as $key) { + // findRevokedBefore() only returns revoked keys, so the date is always set + $revokedAt = $key->getRevokedAt(); + $io->writeln(sprintf('%s%s %s revoked %s', $dryRun ? 'Would delete ' : '', $key->getAppId(), (string) $key->getName(), null === $revokedAt ? '-' : $revokedAt->format('Y-m-d H:i'))); + } + + if ($dryRun) { + $io->note(sprintf('Dry run: %d app key(s) would be deleted.', count($keys))); + + return self::SUCCESS; + } + + $this->repository->delete($keys); + $io->success(sprintf('Deleted %d app key(s).', count($keys))); + + return self::SUCCESS; + } +} diff --git a/src/Entity/AppKey.php b/src/Entity/AppKey.php index acd479e..c73a700 100644 --- a/src/Entity/AppKey.php +++ b/src/Entity/AppKey.php @@ -191,7 +191,7 @@ public function setName(string $name): self return $this; } - public function getAppId(): ?string + public function getAppId(): string { return $this->appId; } diff --git a/src/Repository/AppKeyRepository.php b/src/Repository/AppKeyRepository.php index 6c33ae8..bd18f38 100644 --- a/src/Repository/AppKeyRepository.php +++ b/src/Repository/AppKeyRepository.php @@ -52,6 +52,62 @@ public function touch(AppKey $key, \DateTimeImmutable $now): void ->execute(); } + /** + * @return AppKey[] the keys whose revocation date (or expiry date, which is the same field) is on or + * before the cutoff, oldest first + */ + public function findRevokedBefore(\DateTimeImmutable $cutoff): array + { + return $this->createQueryBuilder('k') + ->where('k.revokedAt IS NOT NULL') + ->andWhere('k.revokedAt <= :cutoff') + ->setParameter('cutoff', $cutoff, 'datetime_immutable') + ->orderBy('k.revokedAt', 'ASC') + ->addOrderBy('k.id', 'ASC') + ->getQuery() + ->getResult(); + } + + /** + * Deletes keys and their usage history, all in one transaction. Like touch() and recordUse(), + * it does not flush(), so other pending changes are not saved along the way. + * + * @param AppKey[] $keys + */ + public function delete(array $keys): void + { + if (!$keys) { + return; + } + + $em = $this->getEntityManager(); + $ids = array_map(fn (AppKey $key) => $key->getId(), $keys); + + $em->wrapInTransaction(function () use ($em, $ids): void { + // The usages are deleted by hand. Their FK has ON DELETE CASCADE, but SQLite only honours + // it with PRAGMA foreign_keys = ON, which DBAL 3 does not set by default (it is an opt-in + // middleware, not enabled here). + $em->createQueryBuilder() + ->delete(AppKeyUsage::class, 'u') + ->where('u.appKey IN (:ids)') + ->setParameter('ids', $ids) + ->getQuery() + ->execute(); + + $em->createQueryBuilder() + ->delete(AppKey::class, 'k') + ->where('k.id IN (:ids)') + ->setParameter('ids', $ids) + ->getQuery() + ->execute(); + }); + + // The entities are gone from the database, so they should not linger in the identity map + foreach ($keys as $key) { + $em->detach($key); + } + } + /** * @param string[] $scope * diff --git a/src/Repository/PrincipalRepository.php b/src/Repository/PrincipalRepository.php index b0c8f45..f8d435f 100644 --- a/src/Repository/PrincipalRepository.php +++ b/src/Repository/PrincipalRepository.php @@ -76,6 +76,7 @@ public function findAllMainPrincipalsWithUserIds(): array ->andWhere('p.isMain = :isMain') ->setParameter('isMain', true) ->setParameter('prefix', Principal::PREFIX) + ->orderBy('p.id', 'ASC') ->getQuery() ->getResult(); } diff --git a/tests/Functional/AppKeysDisabledTest.php b/tests/Functional/AppKeysDisabledTest.php index 1e93da8..899f8ef 100644 --- a/tests/Functional/AppKeysDisabledTest.php +++ b/tests/Functional/AppKeysDisabledTest.php @@ -58,7 +58,7 @@ public function testTheCommandsAreNotAvailable(): void $application = new Application(self::bootKernel()); - foreach (['davis:app-key:create', 'davis:app-key:list', 'davis:app-key:revoke'] as $name) { + foreach (['davis:app-key:create', 'davis:app-key:list', 'davis:app-key:revoke', 'davis:app-key:delete', 'davis:app-key:prune'] as $name) { try { $application->find($name); $this->fail($name.' must not be available'); diff --git a/tests/Functional/Commands/AppKeyCommandsTest.php b/tests/Functional/Commands/AppKeyCommandsTest.php index 8365701..9e48e12 100644 --- a/tests/Functional/Commands/AppKeyCommandsTest.php +++ b/tests/Functional/Commands/AppKeyCommandsTest.php @@ -4,6 +4,7 @@ namespace App\Tests\Functional\Commands; +use App\Command\AppKeyPruneCommand; use App\Entity\AppKey; use App\Services\BasicAuth; use Doctrine\ORM\EntityManagerInterface; @@ -24,12 +25,17 @@ private function runCommand(string $name, array $input = []): CommandTester return $tester; } - private static function keyByAppId(string $appId): AppKey + private static function findKeyByAppId(string $appId): ?AppKey { $em = static::getContainer()->get(EntityManagerInterface::class); $em->clear(); - $key = $em->getRepository(AppKey::class)->findOneByAppId($appId); + return $em->getRepository(AppKey::class)->findOneByAppId($appId); + } + + private static function keyByAppId(string $appId): AppKey + { + $key = self::findKeyByAppId($appId); self::assertNotNull($key, "No app key with app id $appId"); return $key; @@ -40,6 +46,38 @@ private static function countKeys(): int return count(static::getContainer()->get(EntityManagerInterface::class)->getRepository(AppKey::class)->findAll()); } + /** + * Saves a key, with one usage row so that the tests can check the history goes away with it. + */ + private static function persistKey(string $name, ?\DateTimeImmutable $revokedAt): AppKey + { + $em = static::getContainer()->get(EntityManagerInterface::class); + + $key = AppKey::create($name, [AppKey::ALL_USERS], $revokedAt); + $key->issueSecret(); + $em->persist($key); + $em->flush(); + + $connection = $em->getConnection(); + $format = $connection->getDatabasePlatform()->getDateTimeFormatString(); + $connection->insert('app_key_usages', [ + 'app_key_id' => $key->getId(), + 'login' => 'test_user', + 'first_used_at' => (new \DateTimeImmutable('-2 days'))->format($format), + 'last_used_at' => (new \DateTimeImmutable('-1 hour'))->format($format), + ]); + + return $key; + } + + private static function countUsages(AppKey $key): int + { + return (int) static::getContainer()->get(EntityManagerInterface::class)->getConnection()->fetchOne( + 'SELECT COUNT(*) FROM app_key_usages WHERE app_key_id = ?', + [$key->getId()] + ); + } + /** * @return array{0: string, 1: string} the app id and the secret */ @@ -151,4 +189,123 @@ public function testRevoke(): void $this->assertSame(Command::SUCCESS, $tester->getStatusCode()); $this->assertEquals($before, self::keyByAppId('app_revoked')->getRevokedAt()); } + + public function testPruneDeletesTheKeysRevokedLongEnoughAgo(): void + { + self::bootKernel(); + $old = self::persistKey('old', new \DateTimeImmutable('-10 days')); + $recent = self::persistKey('recent', new \DateTimeImmutable('-3 days')); + $active = self::persistKey('active', null); + $expiring = self::persistKey('expiring', new \DateTimeImmutable('+10 days')); + + $tester = $this->runCommand('davis:app-key:prune', ['days' => '7']); + + $this->assertSame(Command::SUCCESS, $tester->getStatusCode()); + $this->assertStringContainsString($old->getAppId().' old revoked', $tester->getDisplay()); + $this->assertNull(self::findKeyByAppId($old->getAppId())); + $this->assertSame(0, self::countUsages($old)); + $this->assertNotNull(self::keyByAppId($recent->getAppId())); + $this->assertSame(1, self::countUsages($recent)); + $this->assertNotNull(self::keyByAppId($active->getAppId())); + $this->assertNotNull(self::keyByAppId($expiring->getAppId())); + } + + public function testPruneWithZeroDaysDeletesEveryRevokedKeyButNotAnExpiry(): void + { + self::bootKernel(); + $justRevoked = self::persistKey('just revoked', new \DateTimeImmutable('-1 minute')); + $active = self::persistKey('active', null); + $expiring = self::persistKey('expiring', new \DateTimeImmutable('+1 hour')); + + $tester = $this->runCommand('davis:app-key:prune', ['days' => '0']); + + $this->assertSame(Command::SUCCESS, $tester->getStatusCode()); + $this->assertNull(self::findKeyByAppId($justRevoked->getAppId())); + // app_revoked comes from the fixtures, revoked a day ago + $this->assertNull(self::findKeyByAppId('app_revoked')); + $this->assertNotNull(self::keyByAppId($active->getAppId())); + $this->assertNotNull(self::keyByAppId($expiring->getAppId())); + $this->assertNotNull(self::keyByAppId('app_test')); + } + + public function testPruneDryRunDeletesNothing(): void + { + self::bootKernel(); + $old = self::persistKey('old', new \DateTimeImmutable('-10 days')); + $before = self::countKeys(); + + $tester = $this->runCommand('davis:app-key:prune', ['days' => '7', '--dry-run' => true]); + + $this->assertSame(Command::SUCCESS, $tester->getStatusCode()); + $this->assertStringContainsString('Would delete '.$old->getAppId(), $tester->getDisplay()); + $this->assertSame($before, self::countKeys()); + $this->assertSame(1, self::countUsages($old)); + } + + public function testPruneWithNothingToDeleteSucceeds(): void + { + self::bootKernel(); + $before = self::countKeys(); + + // the fixtures have one revoked key, a day old + $tester = $this->runCommand('davis:app-key:prune', ['days' => '365']); + + $this->assertSame(Command::SUCCESS, $tester->getStatusCode()); + $this->assertStringContainsString('nothing to delete', $tester->getDisplay()); + $this->assertSame($before, self::countKeys()); + } + + public function testPruneRejectsABadNumberOfDays(): void + { + self::bootKernel(); + $before = self::countKeys(); + + // The last two used to pass ctype_digit() and then blow up in DateInterval (or saturate to PHP_INT_MAX) + foreach (['-1', 'abc', '1.5', '', '1e3', (string) (AppKeyPruneCommand::MAX_DAYS + 1), '99999999999999999999'] as $days) { + $tester = $this->runCommand('davis:app-key:prune', ['days' => $days]); + $this->assertSame(Command::INVALID, $tester->getStatusCode(), $days); + $this->assertStringContainsString('not a number of days', $tester->getDisplay(), $days); + } + + $this->assertSame($before, self::countKeys()); + } + + public function testDeleteRefusesAnActiveKey(): void + { + self::bootKernel(); + $active = self::persistKey('active', null); + $expiring = self::persistKey('expiring', new \DateTimeImmutable('+10 days')); + + foreach ([$active, $expiring] as $key) { + $tester = $this->runCommand('davis:app-key:delete', ['app-id' => $key->getAppId()]); + + $this->assertSame(Command::FAILURE, $tester->getStatusCode()); + $this->assertStringContainsString('davis:app-key:revoke', $tester->getDisplay()); + $this->assertNotNull(self::keyByAppId($key->getAppId())); + $this->assertSame(1, self::countUsages($key)); + } + } + + public function testDeleteRemovesARevokedKeyAndItsUsages(): void + { + self::bootKernel(); + $revoked = self::persistKey('revoked', new \DateTimeImmutable('-1 minute')); + + $tester = $this->runCommand('davis:app-key:delete', ['app-id' => $revoked->getAppId()]); + + $this->assertSame(Command::SUCCESS, $tester->getStatusCode()); + $this->assertNull(self::findKeyByAppId($revoked->getAppId())); + $this->assertSame(0, self::countUsages($revoked)); + } + + public function testDeleteFailsOnAnUnknownKey(): void + { + self::bootKernel(); + $before = self::countKeys(); + + $tester = $this->runCommand('davis:app-key:delete', ['app-id' => 'app_nope']); + + $this->assertSame(Command::FAILURE, $tester->getStatusCode()); + $this->assertSame($before, self::countKeys()); + } } diff --git a/tests/Functional/Controllers/ApiControllerTest.php b/tests/Functional/Controllers/ApiControllerTest.php index 754c53a..e4988fc 100644 --- a/tests/Functional/Controllers/ApiControllerTest.php +++ b/tests/Functional/Controllers/ApiControllerTest.php @@ -16,15 +16,17 @@ class ApiControllerTest extends WebTestCase { - /* - * Helper function to get an existing user ID from the user list + /** + * Fetches the user list and returns the entry of one of the fixture users. The entry is found + * by its username rather than its position: the list is ordered by id, but `test_user2` also + * contains `test_user`, so a positional lookup would silently pick the wrong user if the order + * ever changed. * - * @param int $index Index of the user in the list (0 - first user, 1 - second user) - * @param mixed $client + * @param int $index 0 for test_user, 1 for test_user2 * - * @return int User ID + * @return array{user_id: int, principal_id: int, uri: string, username: string} */ - private function getUserId($client, int $index): int + private function getUser($client, int $index): array { $client->request('GET', '/api/v1/users', [], [], [ 'HTTP_ACCEPT' => 'application/json', @@ -37,35 +39,25 @@ private function getUserId($client, int $index): int $data = json_decode((string) $client->getResponse()->getContent(), true); $this->assertArrayHasKey('data', $data); - $this->assertStringContainsString('test_user', $data['data'][$index]['username']); - return $data['data'][$index]['user_id']; + $username = ['test_user', 'test_user2'][$index]; + foreach ($data['data'] as $user) { + if ($user['username'] === $username) { + return $user; + } + } + + $this->fail($username.' is not in the user list'); } - /* - * Helper function to get an existing username from the user list - * - * @param int $index Index of the user in the list (0 - first user, 1 - second user) - * @param mixed $client - * - * @return string Username - */ - private function getUserUsername($client, int $index): string + private function getUserId($client, int $index): int { - $client->request('GET', '/api/v1/users', [], [], [ - 'HTTP_ACCEPT' => 'application/json', - 'HTTP_X_DAVIS_API_TOKEN' => $_ENV['API_KEY'], - ]); - - $this->assertResponseIsSuccessful(); - $this->assertResponseHeaderSame('Content-Type', 'application/json'); - - $data = json_decode((string) $client->getResponse()->getContent(), true); - - $this->assertArrayHasKey('data', $data); - $this->assertStringContainsString('test_user', $data['data'][$index]['username']); + return $this->getUser($client, $index)['user_id']; + } - return $data['data'][$index]['username']; + private function getUserUsername($client, int $index): string + { + return $this->getUser($client, $index)['username']; } /* diff --git a/tests/Functional/Entity/AppKeyTest.php b/tests/Functional/Entity/AppKeyTest.php index c128705..1879684 100644 --- a/tests/Functional/Entity/AppKeyTest.php +++ b/tests/Functional/Entity/AppKeyTest.php @@ -13,7 +13,7 @@ public function testCreateGeneratesAPrefixedHexAppId(): void { $key = AppKey::create('cal.com', ['*']); - $this->assertMatchesRegularExpression('/^app_[0-9a-f]{24}$/', (string) $key->getAppId()); + $this->assertMatchesRegularExpression('/^app_[0-9a-f]{24}$/', $key->getAppId()); $this->assertNotNull($key->getCreatedAt()); $this->assertSame(['*'], $key->getScope()); $this->assertNull($key->getRevokedAt());