From 57c889f77322a25be6691db768dc4ffd115ce3d7 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 218fa26b998f9..0a972fe2d1b76 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -123,6 +123,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 20ecf5360b7a0..13b84d21963e0 100644 --- a/apps/settings/lib/SetupChecks/CodeIntegrity.php +++ b/apps/settings/lib/SetupChecks/CodeIntegrity.php @@ -50,7 +50,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..c0df3fb330803 --- /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 e8fd087ebc2f9..b17e4b1c0bfd3 100644 --- a/lib/private/IntegrityCheck/Checker.php +++ b/lib/private/IntegrityCheck/Checker.php @@ -396,7 +396,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; } @@ -404,15 +404,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; } /** @@ -422,7 +427,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;