From 8fc498fb820ed1459475eec98cfedb23161dd643 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 27 Jun 2024 15:14:26 +0200 Subject: [PATCH] fix(IntegrityCheck): Ensure the check is run if no results are available If there are no cached results the current implementation was also returning an empty array, but this was the same as when there was a successful run. So to distinguish this we return `null` if there are *no* results. In this case we need to rerun the integrity checker. Signed-off-by: Ferdinand Thiessen --- .../lib/Controller/CheckSetupController.php | 4 + .../lib/SetupChecks/CodeIntegrity.php | 9 +- .../tests/SetupChecks/CodeIntegrityTest.php | 135 ++++++++++++++++++ lib/private/IntegrityCheck/Checker.php | 15 +- 4 files changed, 157 insertions(+), 6 deletions(-) create mode 100644 apps/settings/tests/SetupChecks/CodeIntegrityTest.php diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php index 5cf48d38ffb79..d4e05ec90e4da 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -85,6 +85,10 @@ public function getFailedIntegrityCheckFiles(): DataDisplayResponse { $completeResults = $this->checker->getResults(); + if ($completeResults === null) { + return new DataDisplayResponse('Integrity checker has not been run. Integrity information not available.'); + } + if (!empty($completeResults)) { $formattedTextResponse = 'Technical information ===================== diff --git a/apps/settings/lib/SetupChecks/CodeIntegrity.php b/apps/settings/lib/SetupChecks/CodeIntegrity.php index dc29c4da3061c..2b4271fae9c4c 100644 --- a/apps/settings/lib/SetupChecks/CodeIntegrity.php +++ b/apps/settings/lib/SetupChecks/CodeIntegrity.php @@ -33,7 +33,14 @@ public function getCategory(): string { public function run(): SetupResult { if (!$this->checker->isCodeCheckEnforced()) { return SetupResult::info($this->l10n->t('Integrity checker has been disabled. Integrity cannot be verified.')); - } elseif ($this->checker->hasPassedCheck()) { + } + + // If there are no results we need to run the verification + if ($this->checker->getResults() === null) { + $this->checker->runInstanceVerification(); + } + + if ($this->checker->hasPassedCheck()) { return SetupResult::success($this->l10n->t('No altered files')); } else { return SetupResult::error( diff --git a/apps/settings/tests/SetupChecks/CodeIntegrityTest.php b/apps/settings/tests/SetupChecks/CodeIntegrityTest.php new file mode 100644 index 0000000000000..52101aed901f9 --- /dev/null +++ b/apps/settings/tests/SetupChecks/CodeIntegrityTest.php @@ -0,0 +1,135 @@ +l10n = $this->getMockBuilder(IL10N::class) + ->disableOriginalConstructor()->getMock(); + $this->l10n->expects($this->any()) + ->method('t') + ->willReturnCallback(function ($message, array $replace) { + return vsprintf($message, $replace); + }); + $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->checker = $this->createMock(Checker::class); + } + + public function testSkipOnDisabled(): void { + $this->checker->expects($this->atLeastOnce()) + ->method('isCodeCheckEnforced') + ->willReturn(false); + + $check = new CodeIntegrity( + $this->l10n, + $this->urlGenerator, + $this->checker, + ); + $this->assertEquals(SetupResult::INFO, $check->run()->getSeverity()); + } + + public function testSuccessOnEmptyResults(): void { + $this->checker->expects($this->atLeastOnce()) + ->method('isCodeCheckEnforced') + ->willReturn(true); + $this->checker->expects($this->atLeastOnce()) + ->method('getResults') + ->willReturn([]); + $this->checker->expects(($this->atLeastOnce())) + ->method('hasPassedCheck') + ->willReturn(true); + + $check = new CodeIntegrity( + $this->l10n, + $this->urlGenerator, + $this->checker, + ); + $this->assertEquals(SetupResult::SUCCESS, $check->run()->getSeverity()); + } + + public function testCheckerIsReRunWithoutResults(): void { + $this->checker->expects($this->atLeastOnce()) + ->method('isCodeCheckEnforced') + ->willReturn(true); + $this->checker->expects($this->atLeastOnce()) + ->method('getResults') + ->willReturn(null); + $this->checker->expects(($this->atLeastOnce())) + ->method('hasPassedCheck') + ->willReturn(true); + + // This is important and must be called + $this->checker->expects($this->once()) + ->method('runInstanceVerification'); + + $check = new CodeIntegrity( + $this->l10n, + $this->urlGenerator, + $this->checker, + ); + $this->assertEquals(SetupResult::SUCCESS, $check->run()->getSeverity()); + } + + public function testCheckerIsNotReReInAdvance(): void { + $this->checker->expects($this->atLeastOnce()) + ->method('isCodeCheckEnforced') + ->willReturn(true); + $this->checker->expects($this->atLeastOnce()) + ->method('getResults') + ->willReturn(['mocked']); + $this->checker->expects(($this->atLeastOnce())) + ->method('hasPassedCheck') + ->willReturn(true); + + // There are results thus this must never be called + $this->checker->expects($this->never()) + ->method('runInstanceVerification'); + + $check = new CodeIntegrity( + $this->l10n, + $this->urlGenerator, + $this->checker, + ); + $this->assertEquals(SetupResult::SUCCESS, $check->run()->getSeverity()); + } + + public function testErrorOnMissingIntegrity(): void { + $this->checker->expects($this->atLeastOnce()) + ->method('isCodeCheckEnforced') + ->willReturn(true); + $this->checker->expects($this->atLeastOnce()) + ->method('getResults') + ->willReturn(['mocked']); + $this->checker->expects(($this->atLeastOnce())) + ->method('hasPassedCheck') + ->willReturn(false); + + $check = new CodeIntegrity( + $this->l10n, + $this->urlGenerator, + $this->checker, + ); + $this->assertEquals(SetupResult::ERROR, $check->run()->getSeverity()); + } +} diff --git a/lib/private/IntegrityCheck/Checker.php b/lib/private/IntegrityCheck/Checker.php index a6de3cf6030d7..d38ccf497f4c1 100644 --- a/lib/private/IntegrityCheck/Checker.php +++ b/lib/private/IntegrityCheck/Checker.php @@ -373,7 +373,7 @@ private function verify(string $signaturePath, string $basePath, string $certifi */ public function hasPassedCheck(): bool { $results = $this->getResults(); - if (empty($results)) { + if ($results !== null && empty($results)) { return true; } @@ -381,15 +381,20 @@ public function hasPassedCheck(): bool { } /** - * @return array + * @return array|null Either the results or null if no results available */ - public function getResults(): array { + public function getResults(): array|null { $cachedResults = $this->cache->get(self::CACHE_KEY); if (!\is_null($cachedResults) and $cachedResults !== false) { return json_decode($cachedResults, true); } - return $this->appConfig?->getValueArray('core', self::CACHE_KEY, lazy: true) ?? []; + if ($this->appConfig?->hasKey('core', self::CACHE_KEY, lazy: true)) { + return $this->appConfig->getValueArray('core', self::CACHE_KEY, lazy: true); + } + + // No results available + return null; } /** @@ -399,7 +404,7 @@ public function getResults(): array { * @param array $result */ private function storeResults(string $scope, array $result) { - $resultArray = $this->getResults(); + $resultArray = $this->getResults() ?? []; unset($resultArray[$scope]); if (!empty($result)) { $resultArray[$scope] = $result;