From 90d316018576c7a816340a1cea7eb9f006ce065c 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 | 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 8008351838556..4b8778315327e 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 a5dec637bdba8..d13a8e01c5bbb 100644 --- a/lib/private/IntegrityCheck/Checker.php +++ b/lib/private/IntegrityCheck/Checker.php @@ -427,7 +427,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; } @@ -435,18 +435,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); } - 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 @@ 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;