From 90d316018576c7a816340a1cea7eb9f006ce065c Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Thu, 27 Jun 2024 15:14:26 +0200 Subject: 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 + apps/settings/lib/SetupChecks/CodeIntegrity.php | 9 +- .../tests/SetupChecks/CodeIntegrityTest.php | 135 +++++++++++++++++++++ lib/private/IntegrityCheck/Checker.php | 16 +-- 4 files changed, 156 insertions(+), 8 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 80083518385..4b877831532 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -123,6 +123,10 @@ class CheckSetupController extends Controller { $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 20ecf5360b7..13b84d21963 100644 --- a/apps/settings/lib/SetupChecks/CodeIntegrity.php +++ b/apps/settings/lib/SetupChecks/CodeIntegrity.php @@ -50,7 +50,14 @@ class CodeIntegrity implements ISetupCheck { 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 00000000000..c0df3fb3308 --- /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 a5dec637bdb..d13a8e01c5b 100644 --- a/lib/private/IntegrityCheck/Checker.php +++ b/lib/private/IntegrityCheck/Checker.php @@ -427,7 +427,7 @@ class Checker { */ public function hasPassedCheck(): bool { $results = $this->getResults(); - if (empty($results)) { + if ($results !== null && empty($results)) { return true; } @@ -435,18 +435,20 @@ class Checker { } /** - * @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); } - if ($this->config !== null) { - return json_decode($this->config->getAppValue('core', self::CACHE_KEY, '{}'), true); + $appValue = $this->config?->getAppValue('core', self::CACHE_KEY); + if (!empty($appValue)) { + return json_decode($appValue, true); } - return []; + // No results + return null; } /** @@ -456,7 +458,7 @@ class Checker { * @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; -- cgit v1.2.3