App key deletion and pruning

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 <app_id>`) 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 <app_id>` (an active key must be revoked first). To clean up in bulk, `davis:app-key:prune <days>` deletes, in one go, every key revoked (or expired) at least `<days>` 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 `<days>` 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

psalm-baseline.xml

@@ -209,12 +209,6 @@
       <code><![CDATA[getStatusCode]]></code>
     </PossiblyNullReference>
   </file>
-  <file src="tests/Functional/Controllers/AppKeyControllerTest.php">
-    <PossiblyNullArgument>
-      <code><![CDATA[$onlyUser2->getAppId()]]></code>
-      <code><![CDATA[$user2AndBob->getAppId()]]></code>
-    </PossiblyNullArgument>
-  </file>
   <file src="tests/Functional/Controllers/CalendarControllerTest.php">
     <PossiblyNullReference>
       <code><![CDATA[getId]]></code>

src/Command/AppKeyDeleteCommand.php

@@ -0,0 +1,65 @@
+<?php
+
+namespace App\Command;
+
+use App\Repository\AppKeyRepository;
+use App\Services\AppKeysFeature;
+use Symfony\Component\Console\Attribute\AsCommand;
+use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\Input\InputArgument;
+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Console\Style\SymfonyStyle;
+
+// The description is set in configure(), not here. With a description here, Symfony wraps the
+// command in a LazyCommand, which is always enabled, so isEnabled() below would have no effect.
+#[AsCommand(name: 'davis:app-key:delete')]
+class AppKeyDeleteCommand extends Command
+{
+    public function __construct(
+        private AppKeyRepository $repository,
+        private AppKeysFeature $appKeys,
+    ) {
+        parent::__construct();
+    }
+
+    // When APP_KEYS_ENABLED is off, the command is not registered
+    public function isEnabled(): bool
+    {
+        return $this->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;
+    }
+}

src/Command/AppKeyPruneCommand.php

@@ -0,0 +1,100 @@
+<?php
+
+namespace App\Command;
+
+use App\Repository\AppKeyRepository;
+use App\Services\AppKeysFeature;
+use Symfony\Component\Console\Attribute\AsCommand;
+use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\Input\InputArgument;
+use Symfony\Component\Console\Input\InputInterface;
+use Symfony\Component\Console\Input\InputOption;
+use Symfony\Component\Console\Output\OutputInterface;
+use Symfony\Component\Console\Style\SymfonyStyle;
+
+// The description is set in configure(), not here. With a description here, Symfony wraps the
+// command in a LazyCommand, which is always enabled, so isEnabled() below would have no effect.
+#[AsCommand(name: 'davis:app-key:prune')]
+class AppKeyPruneCommand extends Command
+{
+    // 100 years. PHP cannot build a DateInterval from a much larger number of days anyway.
+    public const MAX_DAYS = 36500;
+
+    public function __construct(
+        private AppKeyRepository $repository,
+        private AppKeysFeature $appKeys,
+    ) {
+        parent::__construct();
+    }
+
+    // When APP_KEYS_ENABLED is off, the command is not registered
+    public function isEnabled(): bool
+    {
+        return $this->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 <info>davis:app-key:revoke</info> first.
+
+                Davis does not run this on its own. To do it every night, for example:
+
+                  <comment>0 3 * * * cd /path/to/davis && bin/console davis:app-key:prune 90 -q</comment>
+
+                Use <info>--dry-run</info> 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;
+    }
+}

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;
     }

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
      *

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();
     }

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');