From e8a070e7f91e5de95c4ab54a7f8a67d8bfee96c2 Mon Sep 17 00:00:00 2001 From: tchapi Date: Thu, 5 Nov 2026 20:08:55 +0100 Subject: [PATCH 1/4] chore --- README.md | 20 ++- src/Command/AppKeyDeleteCommand.php | 67 ++++++++ src/Command/AppKeyPruneCommand.php | 95 +++++++++++ src/Repository/AppKeyRepository.php | 37 +++++ tests/Functional/AppKeysDisabledTest.php | 2 +- .../Commands/AppKeyCommandsTest.php | 149 ++++++++++++++++++ 6 files changed, 366 insertions(+), 4 deletions(-) create mode 100644 src/Command/AppKeyDeleteCommand.php create mode 100644 src/Command/AppKeyPruneCommand.php diff --git a/README.md b/README.md index 3d2fc897..fc822d33 100644 --- a/README.md +++ b/README.md @@ -169,6 +169,14 @@ AUTH_METHOD=Basic # can be "Basic", "IMAP" or "LDAP" > one is stored with bcrypt and no longer depends on the realm. Your own admin login is unaffected — > it comes from `ADMIN_LOGIN` / `ADMIN_PASSWORD`, not from the users table. +**Update check** + +```shell +UPDATE_CHECK_ENABLED=false +``` + +Davis does not phone home. Set `UPDATE_CHECK_ENABLED=true` to have the [diagnostics page](#start-with-the-diagnostics-page) ask GitHub whether a newer release exists, and link to its release notes when there is one. The dashboard then counts it among the points that need your attention. GitHub (`api.github.com`) is asked at most once a day, anonymously, and only when you open the dashboard; nothing about your install is sent. If GitHub cannot be reached, the check says so and is tried again an hour later. + **App keys** ```shell @@ -191,13 +199,19 @@ 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). -**Update check** +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 every key revoked 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 -UPDATE_CHECK_ENABLED=false +0 3 * * * cd /path/to/davis && bin/console davis:app-key:prune 90 -q ``` -Davis does not phone home. Set `UPDATE_CHECK_ENABLED=true` to have the [diagnostics page](#start-with-the-diagnostics-page) ask GitHub whether a newer release exists, and link to its release notes when there is one. The dashboard then counts it among the points that need your attention. GitHub (`api.github.com`) is asked at most once a day, anonymously, and only when you open the dashboard; nothing about your install is sent. If GitHub cannot be reached, the check says so and is tried again an hour later. +or, with the docker image: + +```shell +0 3 * * * docker exec davis sh -c "APP_ENV=prod bin/console davis:app-key:prune 90 -q" +``` **d. The global flags to enable CalDAV, CardDAV and WebDAV**. You can also disable the option to have calendars public diff --git a/src/Command/AppKeyDeleteCommand.php b/src/Command/AppKeyDeleteCommand.php new file mode 100644 index 00000000..97533fbb --- /dev/null +++ b/src/Command/AppKeyDeleteCommand.php @@ -0,0 +1,67 @@ +appKeys->isEnabled(); + } + + protected function configure(): void + { + $this + ->setDescription('Delete a revoked 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 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'); + + $repository = $this->doctrine->getRepository(AppKey::class); + $key = $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; + } + + $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 00000000..6588ab8b --- /dev/null +++ b/src/Command/AppKeyPruneCommand.php @@ -0,0 +1,95 @@ +appKeys->isEnabled(); + } + + protected function configure(): void + { + $this + ->setDescription('Delete the app keys revoked a while ago') + ->addArgument('days', InputArgument::REQUIRED, 'Delete the keys revoked at least this many days ago (0 for every revoked key)') + ->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 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. + + A key whose revocation date is in the future (an expiry) is still 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'); + + if (!ctype_digit($days)) { + $io->error(sprintf('"%s" is not a number of days, expected an integer >= 0.', $days)); + + return self::INVALID; + } + + $dryRun = $input->getOption('dry-run'); + $cutoff = (new \DateTimeImmutable())->sub(new \DateInterval(sprintf('P%dD', (int) $days))); + + $repository = $this->doctrine->getRepository(AppKey::class); + $keys = $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) { + $io->writeln(sprintf('%s %s revoked %s', $key->getAppId(), $key->getName(), $key->getRevokedAt()->format('Y-m-d H:i'))); + + if (!$dryRun) { + $repository->delete($key); + } + } + + if ($dryRun) { + $io->note(sprintf('Dry run: %d app key(s) would be deleted.', count($keys))); + } else { + $io->success(sprintf('Deleted %d app key(s).', count($keys))); + } + + return self::SUCCESS; + } +} diff --git a/src/Repository/AppKeyRepository.php b/src/Repository/AppKeyRepository.php index 6c33ae85..55bd52f6 100644 --- a/src/Repository/AppKeyRepository.php +++ b/src/Repository/AppKeyRepository.php @@ -52,6 +52,43 @@ public function touch(AppKey $key, \DateTimeImmutable $now): void ->execute(); } + /** + * @return AppKey[] the keys revoked (or expired) on or before the cutoff, oldest revocation 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 a key and its usage history. + */ + public function delete(AppKey $key): void + { + $em = $this->getEntityManager(); + + $em->wrapInTransaction(function () use ($em, $key): 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. + $em->createQueryBuilder() + ->delete(AppKeyUsage::class, 'u') + ->where('u.appKey = :key') + ->setParameter('key', $key) + ->getQuery() + ->execute(); + + $em->remove($key); + $em->flush(); + }); + } + /** * @param string[] $scope * diff --git a/tests/Functional/AppKeysDisabledTest.php b/tests/Functional/AppKeysDisabledTest.php index 1e93da86..899f8ef4 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 83657010..213c3ef1 100644 --- a/tests/Functional/Commands/AppKeyCommandsTest.php +++ b/tests/Functional/Commands/AppKeyCommandsTest.php @@ -40,6 +40,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 +183,121 @@ 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::keyByAppId($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::keyByAppId($justRevoked->getAppId())); + // app_revoked comes from the fixtures, revoked a day ago + $this->assertNull(self::keyByAppId('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($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(); + + foreach (['-1', 'abc', '1.5', ''] as $days) { + $tester = $this->runCommand('davis:app-key:prune', ['days' => $days]); + $this->assertSame(Command::INVALID, $tester->getStatusCode(), $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::keyByAppId($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()); + } } From 58a1ea0d316eef4154ab8594742f706ab03fbc7a Mon Sep 17 00:00:00 2001 From: tchapi Date: Mon, 9 Nov 2026 12:10:22 +0100 Subject: [PATCH 2/4] Fixes and improvements --- README.md | 2 +- src/Command/AppKeyDeleteCommand.php | 6 +-- src/Command/AppKeyPruneCommand.php | 39 ++++++++------ src/Repository/AppKeyRepository.php | 39 ++++++++++---- src/Repository/PrincipalRepository.php | 1 + .../Commands/AppKeyCommandsTest.php | 7 ++- .../Controllers/ApiControllerTest.php | 52 ++++++++----------- 7 files changed, 83 insertions(+), 63 deletions(-) diff --git a/README.md b/README.md index fc822d33..2d04f7e6 100644 --- a/README.md +++ b/README.md @@ -199,7 +199,7 @@ 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 every key revoked 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. +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: diff --git a/src/Command/AppKeyDeleteCommand.php b/src/Command/AppKeyDeleteCommand.php index 97533fbb..ce0a8f3a 100644 --- a/src/Command/AppKeyDeleteCommand.php +++ b/src/Command/AppKeyDeleteCommand.php @@ -33,9 +33,9 @@ public function isEnabled(): bool protected function configure(): void { $this - ->setDescription('Delete a revoked app key') + ->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 app key for good, with its usage history. An active key (including one with an expiry date still ahead) must be revoked first.'); + ->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 @@ -58,7 +58,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int return self::FAILURE; } - $repository->delete($key); + $repository->delete([$key]); $io->success(sprintf('Deleted %s.', $appId)); diff --git a/src/Command/AppKeyPruneCommand.php b/src/Command/AppKeyPruneCommand.php index 6588ab8b..731036ff 100644 --- a/src/Command/AppKeyPruneCommand.php +++ b/src/Command/AppKeyPruneCommand.php @@ -18,6 +18,9 @@ #[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 ManagerRegistry $doctrine, private AppKeysFeature $appKeys, @@ -34,16 +37,17 @@ public function isEnabled(): bool protected function configure(): void { $this - ->setDescription('Delete the app keys revoked a while ago') - ->addArgument('days', InputArgument::REQUIRED, 'Delete the keys revoked at least this many days ago (0 for every revoked key)') + ->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 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. + 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 revocation date is in the future (an expiry) is still active and is never touched. - Keys that were never revoked are not touched either, use davis:app-key:revoke first. + 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: @@ -58,14 +62,16 @@ protected function execute(InputInterface $input, OutputInterface $output): int $io = new SymfonyStyle($input, $output); $days = $input->getArgument('days'); - if (!ctype_digit($days)) { - $io->error(sprintf('"%s" is not a number of days, expected an integer >= 0.', $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', (int) $days))); + $cutoff = (new \DateTimeImmutable())->sub(new \DateInterval(sprintf('P%dD', $days))); $repository = $this->doctrine->getRepository(AppKey::class); $keys = $repository->findRevokedBefore($cutoff); @@ -77,19 +83,18 @@ protected function execute(InputInterface $input, OutputInterface $output): int } foreach ($keys as $key) { - $io->writeln(sprintf('%s %s revoked %s', $key->getAppId(), $key->getName(), $key->getRevokedAt()->format('Y-m-d H:i'))); - - if (!$dryRun) { - $repository->delete($key); - } + $io->writeln(sprintf('%s%s %s revoked %s', $dryRun ? 'Would delete ' : '', $key->getAppId(), $key->getName(), $key->getRevokedAt()->format('Y-m-d H:i'))); } if ($dryRun) { $io->note(sprintf('Dry run: %d app key(s) would be deleted.', count($keys))); - } else { - $io->success(sprintf('Deleted %d app key(s).', count($keys))); + + return self::SUCCESS; } + $repository->delete($keys); + $io->success(sprintf('Deleted %d app key(s).', count($keys))); + return self::SUCCESS; } } diff --git a/src/Repository/AppKeyRepository.php b/src/Repository/AppKeyRepository.php index 55bd52f6..bd18f38b 100644 --- a/src/Repository/AppKeyRepository.php +++ b/src/Repository/AppKeyRepository.php @@ -53,7 +53,8 @@ public function touch(AppKey $key, \DateTimeImmutable $now): void } /** - * @return AppKey[] the keys revoked (or expired) on or before the cutoff, oldest revocation first + * @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 { @@ -68,25 +69,43 @@ public function findRevokedBefore(\DateTimeImmutable $cutoff): array } /** - * Deletes a key and its usage history. + * 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(AppKey $key): void + 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, $key): 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. + $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 = :key') - ->setParameter('key', $key) + ->where('u.appKey IN (:ids)') + ->setParameter('ids', $ids) ->getQuery() ->execute(); - $em->remove($key); - $em->flush(); + $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); + } } /** diff --git a/src/Repository/PrincipalRepository.php b/src/Repository/PrincipalRepository.php index b0c8f45f..f8d435fd 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/Commands/AppKeyCommandsTest.php b/tests/Functional/Commands/AppKeyCommandsTest.php index 213c3ef1..ceb37b5f 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; @@ -231,7 +232,7 @@ public function testPruneDryRunDeletesNothing(): void $tester = $this->runCommand('davis:app-key:prune', ['days' => '7', '--dry-run' => true]); $this->assertSame(Command::SUCCESS, $tester->getStatusCode()); - $this->assertStringContainsString($old->getAppId(), $tester->getDisplay()); + $this->assertStringContainsString('Would delete '.$old->getAppId(), $tester->getDisplay()); $this->assertSame($before, self::countKeys()); $this->assertSame(1, self::countUsages($old)); } @@ -254,9 +255,11 @@ public function testPruneRejectsABadNumberOfDays(): void self::bootKernel(); $before = self::countKeys(); - foreach (['-1', 'abc', '1.5', ''] as $days) { + // 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()); diff --git a/tests/Functional/Controllers/ApiControllerTest.php b/tests/Functional/Controllers/ApiControllerTest.php index 754c53ab..e4988fcc 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']; } /* From 4a8ee5e51befb62810ee5048e0adcf7adf2bd116 Mon Sep 17 00:00:00 2001 From: tchapi Date: Mon, 9 Nov 2026 20:31:35 +0100 Subject: [PATCH 3/4] Fix rebase --- psalm-baseline.xml | 6 ------ src/Command/AppKeyDeleteCommand.php | 10 ++++------ src/Command/AppKeyPruneCommand.php | 14 +++++++------- src/Entity/AppKey.php | 2 +- .../Functional/Commands/AppKeyCommandsTest.php | 17 +++++++++++------ tests/Functional/Entity/AppKeyTest.php | 2 +- 6 files changed, 24 insertions(+), 27 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 2654d1fe..9cb8fdd2 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 index ce0a8f3a..524f622f 100644 --- a/src/Command/AppKeyDeleteCommand.php +++ b/src/Command/AppKeyDeleteCommand.php @@ -2,9 +2,8 @@ namespace App\Command; -use App\Entity\AppKey; +use App\Repository\AppKeyRepository; use App\Services\AppKeysFeature; -use Doctrine\Persistence\ManagerRegistry; use Symfony\Component\Console\Attribute\AsCommand; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -18,7 +17,7 @@ class AppKeyDeleteCommand extends Command { public function __construct( - private ManagerRegistry $doctrine, + private AppKeyRepository $repository, private AppKeysFeature $appKeys, ) { parent::__construct(); @@ -43,8 +42,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $io = new SymfonyStyle($input, $output); $appId = $input->getArgument('app-id'); - $repository = $this->doctrine->getRepository(AppKey::class); - $key = $repository->findOneByAppId($appId); + $key = $this->repository->findOneByAppId($appId); if (null === $key) { $io->error(sprintf('No app key with the app id %s.', $appId)); @@ -58,7 +56,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int return self::FAILURE; } - $repository->delete([$key]); + $this->repository->delete([$key]); $io->success(sprintf('Deleted %s.', $appId)); diff --git a/src/Command/AppKeyPruneCommand.php b/src/Command/AppKeyPruneCommand.php index 731036ff..13dd787e 100644 --- a/src/Command/AppKeyPruneCommand.php +++ b/src/Command/AppKeyPruneCommand.php @@ -2,9 +2,8 @@ namespace App\Command; -use App\Entity\AppKey; +use App\Repository\AppKeyRepository; use App\Services\AppKeysFeature; -use Doctrine\Persistence\ManagerRegistry; use Symfony\Component\Console\Attribute\AsCommand; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -22,7 +21,7 @@ class AppKeyPruneCommand extends Command public const MAX_DAYS = 36500; public function __construct( - private ManagerRegistry $doctrine, + private AppKeyRepository $repository, private AppKeysFeature $appKeys, ) { parent::__construct(); @@ -73,8 +72,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $dryRun = $input->getOption('dry-run'); $cutoff = (new \DateTimeImmutable())->sub(new \DateInterval(sprintf('P%dD', $days))); - $repository = $this->doctrine->getRepository(AppKey::class); - $keys = $repository->findRevokedBefore($cutoff); + $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'))); @@ -83,7 +81,9 @@ protected function execute(InputInterface $input, OutputInterface $output): int } foreach ($keys as $key) { - $io->writeln(sprintf('%s%s %s revoked %s', $dryRun ? 'Would delete ' : '', $key->getAppId(), $key->getName(), $key->getRevokedAt()->format('Y-m-d H:i'))); + // 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) { @@ -92,7 +92,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int return self::SUCCESS; } - $repository->delete($keys); + $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 acd479ef..c73a7004 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/tests/Functional/Commands/AppKeyCommandsTest.php b/tests/Functional/Commands/AppKeyCommandsTest.php index ceb37b5f..9e48e126 100644 --- a/tests/Functional/Commands/AppKeyCommandsTest.php +++ b/tests/Functional/Commands/AppKeyCommandsTest.php @@ -25,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; @@ -197,7 +202,7 @@ public function testPruneDeletesTheKeysRevokedLongEnoughAgo(): void $this->assertSame(Command::SUCCESS, $tester->getStatusCode()); $this->assertStringContainsString($old->getAppId().' old revoked', $tester->getDisplay()); - $this->assertNull(self::keyByAppId($old->getAppId())); + $this->assertNull(self::findKeyByAppId($old->getAppId())); $this->assertSame(0, self::countUsages($old)); $this->assertNotNull(self::keyByAppId($recent->getAppId())); $this->assertSame(1, self::countUsages($recent)); @@ -215,9 +220,9 @@ public function testPruneWithZeroDaysDeletesEveryRevokedKeyButNotAnExpiry(): voi $tester = $this->runCommand('davis:app-key:prune', ['days' => '0']); $this->assertSame(Command::SUCCESS, $tester->getStatusCode()); - $this->assertNull(self::keyByAppId($justRevoked->getAppId())); + $this->assertNull(self::findKeyByAppId($justRevoked->getAppId())); // app_revoked comes from the fixtures, revoked a day ago - $this->assertNull(self::keyByAppId('app_revoked')); + $this->assertNull(self::findKeyByAppId('app_revoked')); $this->assertNotNull(self::keyByAppId($active->getAppId())); $this->assertNotNull(self::keyByAppId($expiring->getAppId())); $this->assertNotNull(self::keyByAppId('app_test')); @@ -289,7 +294,7 @@ public function testDeleteRemovesARevokedKeyAndItsUsages(): void $tester = $this->runCommand('davis:app-key:delete', ['app-id' => $revoked->getAppId()]); $this->assertSame(Command::SUCCESS, $tester->getStatusCode()); - $this->assertNull(self::keyByAppId($revoked->getAppId())); + $this->assertNull(self::findKeyByAppId($revoked->getAppId())); $this->assertSame(0, self::countUsages($revoked)); } diff --git a/tests/Functional/Entity/AppKeyTest.php b/tests/Functional/Entity/AppKeyTest.php index c128705a..18796842 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()); From 0d3fd0e66b815026ec756b55e5975a2abcd386c6 Mon Sep 17 00:00:00 2001 From: tchapi Date: Mon, 9 Nov 2026 21:04:03 +0100 Subject: [PATCH 4/4] Fix rebase --- README.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 2d04f7e6..6b13c2fe 100644 --- a/README.md +++ b/README.md @@ -169,14 +169,6 @@ AUTH_METHOD=Basic # can be "Basic", "IMAP" or "LDAP" > one is stored with bcrypt and no longer depends on the realm. Your own admin login is unaffected — > it comes from `ADMIN_LOGIN` / `ADMIN_PASSWORD`, not from the users table. -**Update check** - -```shell -UPDATE_CHECK_ENABLED=false -``` - -Davis does not phone home. Set `UPDATE_CHECK_ENABLED=true` to have the [diagnostics page](#start-with-the-diagnostics-page) ask GitHub whether a newer release exists, and link to its release notes when there is one. The dashboard then counts it among the points that need your attention. GitHub (`api.github.com`) is asked at most once a day, anonymously, and only when you open the dashboard; nothing about your install is sent. If GitHub cannot be reached, the check says so and is tried again an hour later. - **App keys** ```shell @@ -213,6 +205,14 @@ or, with the docker image: 0 3 * * * docker exec davis sh -c "APP_ENV=prod bin/console davis:app-key:prune 90 -q" ``` +**Update check** + +```shell +UPDATE_CHECK_ENABLED=false +``` + +Davis does not phone home. Set `UPDATE_CHECK_ENABLED=true` to have the [diagnostics page](#start-with-the-diagnostics-page) ask GitHub whether a newer release exists, and link to its release notes when there is one. The dashboard then counts it among the points that need your attention. GitHub (`api.github.com`) is asked at most once a day, anonymously, and only when you open the dashboard; nothing about your install is sent. If GitHub cannot be reached, the check says so and is tried again an hour later. + **d. The global flags to enable CalDAV, CardDAV and WebDAV**. You can also disable the option to have calendars public ```shell