diff --git a/Classes/Command/CleanupIpLogCommand.php b/Classes/Command/CleanupIpLogCommand.php index e6f2284..1deb26d 100644 --- a/Classes/Command/CleanupIpLogCommand.php +++ b/Classes/Command/CleanupIpLogCommand.php @@ -59,12 +59,22 @@ protected function execute(InputInterface $input, OutputInterface $output): int $threshold = time() - $days * self::SECONDS_PER_DAY; if (true === $input->getOption('dry-run')) { + $missing = $this->ipLogRepository->countEntriesWithMissingTimestamp(); + if ($missing > 0) { + $io->writeln(sprintf('%d legacy IP log entries without a last-seen timestamp would be initialized.', $missing)); + } + $count = $this->ipLogRepository->countEntriesLastSeenBefore($threshold); $io->writeln(sprintf('%d IP log entries would be deleted (not seen for more than %d days).', $count, $days)); return Command::SUCCESS; } + $initialized = $this->ipLogRepository->initializeMissingTimestamps(); + if ($initialized > 0) { + $io->writeln(sprintf('Initialized %d legacy IP log entries without a last-seen timestamp.', $initialized)); + } + $deleted = $this->ipLogRepository->deleteEntriesLastSeenBefore($threshold); $io->writeln(sprintf('Deleted %d IP log entries (not seen for more than %d days).', $deleted, $days)); diff --git a/Classes/Domain/Repository/IpLogRepository.php b/Classes/Domain/Repository/IpLogRepository.php index 32f9f5b..d3f21b2 100644 --- a/Classes/Domain/Repository/IpLogRepository.php +++ b/Classes/Domain/Repository/IpLogRepository.php @@ -65,6 +65,10 @@ public function registerIdentifier(string $identifierHash): bool } /** + * Entries with tstamp = 0 stem from extension versions that did not track the + * last sighting. Their real age is unknown, so they are excluded here and must + * be initialized via initializeMissingTimestamps() before any cleanup. + * * @throws Exception */ public function countEntriesLastSeenBefore(int $timestamp): int @@ -76,6 +80,7 @@ public function countEntriesLastSeenBefore(int $timestamp): int ->from(self::TABLE_NAME) ->where( $queryBuilder->expr()->lt('tstamp', $queryBuilder->createNamedParameter($timestamp, Connection::PARAM_INT)), + $queryBuilder->expr()->gt('tstamp', $queryBuilder->createNamedParameter(0, Connection::PARAM_INT)), ) ->executeQuery()->fetchOne(); } @@ -88,7 +93,41 @@ public function deleteEntriesLastSeenBefore(int $timestamp): int ->delete(self::TABLE_NAME) ->where( $queryBuilder->expr()->lt('tstamp', $queryBuilder->createNamedParameter($timestamp, Connection::PARAM_INT)), + $queryBuilder->expr()->gt('tstamp', $queryBuilder->createNamedParameter(0, Connection::PARAM_INT)), ) ->executeStatement(); } + + /** + * @throws Exception + */ + public function countEntriesWithMissingTimestamp(): int + { + $queryBuilder = $this->connectionPool->getQueryBuilderForTable(self::TABLE_NAME); + + return (int) $queryBuilder + ->count('uid') + ->from(self::TABLE_NAME) + ->where( + $queryBuilder->expr()->eq('tstamp', $queryBuilder->createNamedParameter(0, Connection::PARAM_INT)), + ) + ->executeQuery()->fetchOne(); + } + + /** + * Backfills the last-seen timestamp of legacy entries with the current time, + * granting them a full retention period before they become cleanup candidates. + * + * @throws Exception + */ + public function initializeMissingTimestamps(): int + { + $connection = $this->connectionPool->getConnectionForTable(self::TABLE_NAME); + + return $connection->update( + self::TABLE_NAME, + ['tstamp' => time()], + ['tstamp' => 0], + ); + } } diff --git a/README.md b/README.md index bc6cbf9..a0cfd39 100644 --- a/README.md +++ b/README.md @@ -116,6 +116,8 @@ Use `--dry-run` to only report how many entries would be deleted. The command is > [!NOTE] > After an entry has been deleted, the next login from that IP address triggers a new-IP notification again — that is the intended effect of a retention period. +Entries created by older extension versions (1.0.3 and below) may lack a last-seen timestamp. The cleanup command initializes such legacy entries with the current time instead of deleting them, so they are granted a full retention period before becoming cleanup candidates — otherwise every already-known IP would be reported as new again after the first cleanup run. + #### Geolocation If `Fetch Geolocation` is enabled, the extension will use the [ip-api.com](https://ip-api.com/) service to fetch geolocation information for the IP address. Only public IP addresses will be looked up to respect privacy. diff --git a/Tests/Unit/Command/CleanupIpLogCommandTest.php b/Tests/Unit/Command/CleanupIpLogCommandTest.php index eea2dc4..6dd25fb 100644 --- a/Tests/Unit/Command/CleanupIpLogCommandTest.php +++ b/Tests/Unit/Command/CleanupIpLogCommandTest.php @@ -90,6 +90,84 @@ public function testDryRunOnlyCountsEntries(): void self::assertStringContainsString('7 IP log entries would be deleted', $this->commandTester->getDisplay()); } + public function testInitializesLegacyEntriesBeforeDeleting(): void + { + $this->ipLogRepository + ->expects(self::once()) + ->method('initializeMissingTimestamps') + ->willReturn(3); + + $this->ipLogRepository + ->expects(self::once()) + ->method('deleteEntriesLastSeenBefore') + ->willReturn(5); + + $exitCode = $this->commandTester->execute([]); + + self::assertSame(Command::SUCCESS, $exitCode); + $display = $this->commandTester->getDisplay(); + self::assertStringContainsString('Initialized 3 legacy IP log entries', $display); + self::assertStringContainsString('Deleted 5 IP log entries', $display); + } + + public function testDoesNotReportInitializationWhenNoLegacyEntriesExist(): void + { + $this->ipLogRepository + ->method('initializeMissingTimestamps') + ->willReturn(0); + + $this->ipLogRepository + ->method('deleteEntriesLastSeenBefore') + ->willReturn(5); + + $this->commandTester->execute([]); + + self::assertStringNotContainsString('Initialized', $this->commandTester->getDisplay()); + } + + public function testDryRunReportsLegacyEntriesWithoutModifying(): void + { + $this->ipLogRepository + ->expects(self::once()) + ->method('countEntriesWithMissingTimestamp') + ->willReturn(4); + + $this->ipLogRepository + ->expects(self::once()) + ->method('countEntriesLastSeenBefore') + ->willReturn(7); + + $this->ipLogRepository + ->expects(self::never()) + ->method('initializeMissingTimestamps'); + + $this->ipLogRepository + ->expects(self::never()) + ->method('deleteEntriesLastSeenBefore'); + + $exitCode = $this->commandTester->execute(['--dry-run' => true]); + + self::assertSame(Command::SUCCESS, $exitCode); + $display = $this->commandTester->getDisplay(); + self::assertStringContainsString('4 legacy IP log entries without a last-seen timestamp would be initialized', $display); + self::assertStringContainsString('7 IP log entries would be deleted', $display); + } + + public function testDryRunDoesNotReportInitializationWhenNoLegacyEntriesExist(): void + { + $this->ipLogRepository + ->method('countEntriesWithMissingTimestamp') + ->willReturn(0); + + $this->ipLogRepository + ->method('countEntriesLastSeenBefore') + ->willReturn(7); + + $this->commandTester->execute(['--dry-run' => true]); + + self::assertStringNotContainsString('would be initialized', $this->commandTester->getDisplay()); + } + public function testFailsForNonPositiveDays(): void { $this->ipLogRepository diff --git a/Tests/Unit/Domain/Repository/IpLogRepositoryTest.php b/Tests/Unit/Domain/Repository/IpLogRepositoryTest.php index 9c398f9..761f9cd 100644 --- a/Tests/Unit/Domain/Repository/IpLogRepositoryTest.php +++ b/Tests/Unit/Domain/Repository/IpLogRepositoryTest.php @@ -114,20 +114,23 @@ public function testRegisterIdentifierReturnsFalseWhenConcurrentLoginInsertedFir self::assertFalse($this->subject->registerIdentifier($identifierHash)); } - public function testCountEntriesLastSeenBefore(): void + public function testCountEntriesLastSeenBeforeExcludesEntriesWithoutTimestamp(): void { $this->queryBuilder->expects(self::once()) ->method('count') ->with('uid') ->willReturnSelf(); $this->queryBuilder->method('from')->willReturnSelf(); - $this->queryBuilder->expects(self::once()) - ->method('createNamedParameter') - ->with(12345, Connection::PARAM_INT) - ->willReturn(':tstamp'); - $this->expressionBuilder->method('lt') - ->with('tstamp', ':tstamp') - ->willReturn('tstamp < :tstamp'); + $this->queryBuilder->method('createNamedParameter') + ->willReturnCallback(static fn (int $value): string => ':param'.$value); + $this->expressionBuilder->expects(self::once()) + ->method('lt') + ->with('tstamp', ':param12345') + ->willReturn('tstamp < :param12345'); + $this->expressionBuilder->expects(self::once()) + ->method('gt') + ->with('tstamp', ':param0') + ->willReturn('tstamp > :param0'); $this->queryBuilder->method('where')->willReturnSelf(); $result = $this->createMock(Result::class); @@ -142,19 +145,22 @@ public function testCountEntriesLastSeenBefore(): void self::assertSame(3, $this->subject->countEntriesLastSeenBefore(12345)); } - public function testDeleteEntriesLastSeenBefore(): void + public function testDeleteEntriesLastSeenBeforeExcludesEntriesWithoutTimestamp(): void { $this->queryBuilder->expects(self::once()) ->method('delete') ->with('tx_typo3loginwarning_iplog') ->willReturnSelf(); - $this->queryBuilder->expects(self::once()) - ->method('createNamedParameter') - ->with(12345, Connection::PARAM_INT) - ->willReturn(':tstamp'); - $this->expressionBuilder->method('lt') - ->with('tstamp', ':tstamp') - ->willReturn('tstamp < :tstamp'); + $this->queryBuilder->method('createNamedParameter') + ->willReturnCallback(static fn (int $value): string => ':param'.$value); + $this->expressionBuilder->expects(self::once()) + ->method('lt') + ->with('tstamp', ':param12345') + ->willReturn('tstamp < :param12345'); + $this->expressionBuilder->expects(self::once()) + ->method('gt') + ->with('tstamp', ':param0') + ->willReturn('tstamp > :param0'); $this->queryBuilder->method('where')->willReturnSelf(); $this->queryBuilder->expects(self::once()) @@ -163,4 +169,46 @@ public function testDeleteEntriesLastSeenBefore(): void self::assertSame(7, $this->subject->deleteEntriesLastSeenBefore(12345)); } + + public function testCountEntriesWithMissingTimestamp(): void + { + $this->queryBuilder->expects(self::once()) + ->method('count') + ->with('uid') + ->willReturnSelf(); + $this->queryBuilder->method('from')->willReturnSelf(); + $this->queryBuilder->expects(self::once()) + ->method('createNamedParameter') + ->with(0, Connection::PARAM_INT) + ->willReturn(':zero'); + $this->expressionBuilder->method('eq') + ->with('tstamp', ':zero') + ->willReturn('tstamp = :zero'); + $this->queryBuilder->method('where')->willReturnSelf(); + + $result = $this->createMock(Result::class); + $result->expects(self::once()) + ->method('fetchOne') + ->willReturn('4'); + + $this->queryBuilder->expects(self::once()) + ->method('executeQuery') + ->willReturn($result); + + self::assertSame(4, $this->subject->countEntriesWithMissingTimestamp()); + } + + public function testInitializeMissingTimestampsSetsCurrentTimeAndReturnsAffectedRows(): void + { + $this->connection->expects(self::once()) + ->method('update') + ->with( + 'tx_typo3loginwarning_iplog', + self::callback(static fn (array $data): bool => is_int($data['tstamp']) && $data['tstamp'] > 0), + ['tstamp' => 0], + ) + ->willReturn(4); + + self::assertSame(4, $this->subject->initializeMissingTimestamps()); + } }