Skip to content

Commit 90d3160

Browse files
susnuxsusnux
committed
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 <opensource@fthiessen.de>
1 parent 23cc6cf commit 90d3160

File tree

4 files changed

+156
-8
lines changed

4 files changed

+156
-8
lines changed

‎apps/settings/lib/Controller/CheckSetupController.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,10 @@ public function getFailedIntegrityCheckFiles(): DataDisplayResponse {
123123

124124
$completeResults = $this->checker->getResults();
125125

126+
if ($completeResults === null) {
127+
return new DataDisplayResponse('Integrity checker has not been run. Integrity information not available.');
128+
}
129+
126130
if (!empty($completeResults)) {
127131
$formattedTextResponse = 'Technical information
128132
=====================

‎apps/settings/lib/SetupChecks/CodeIntegrity.php‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,14 @@ public function getCategory(): string {
5050
public function run(): SetupResult {
5151
if (!$this->checker->isCodeCheckEnforced()) {
5252
return SetupResult::info($this->l10n->t('Integrity checker has been disabled. Integrity cannot be verified.'));
53-
} elseif ($this->checker->hasPassedCheck()) {
53+
}
54+
55+
// If there are no results we need to run the verification
56+
if ($this->checker->getResults() === null) {
57+
$this->checker->runInstanceVerification();
58+
}
59+
60+
if ($this->checker->hasPassedCheck()) {
5461
return SetupResult::success($this->l10n->t('No altered files'));
5562
} else {
5663
return SetupResult::error(

‎apps/settings/tests/SetupChecks/CodeIntegrityTest.php‎

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
namespace OCA\Settings\Tests;
10+
11+
use OC\IntegrityCheck\Checker;
12+
use OCA\Settings\SetupChecks\CodeIntegrity;
13+
use OCP\IL10N;
14+
use OCP\IURLGenerator;
15+
use OCP\SetupCheck\SetupResult;
16+
use PHPUnit\Framework\MockObject\MockObject;
17+
use Test\TestCase;
18+
19+
class CodeIntegrityTest extends TestCase {
20+
21+
private IL10N|MockObject $l10n;
22+
private IURLGenerator|MockObject $urlGenerator;
23+
private Checker|MockObject $checker;
24+
25+
protected function setUp(): void {
26+
parent::setUp();
27+
28+
$this->l10n = $this->getMockBuilder(IL10N::class)
29+
->disableOriginalConstructor()->getMock();
30+
$this->l10n->expects($this->any())
31+
->method('t')
32+
->willReturnCallback(function ($message, array $replace) {
33+
return vsprintf($message, $replace);
34+
});
35+
$this->urlGenerator = $this->createMock(IURLGenerator::class);
36+
$this->checker = $this->createMock(Checker::class);
37+
}
38+
39+
public function testSkipOnDisabled(): void {
40+
$this->checker->expects($this->atLeastOnce())
41+
->method('isCodeCheckEnforced')
42+
->willReturn(false);
43+
44+
$check = new CodeIntegrity(
45+
$this->l10n,
46+
$this->urlGenerator,
47+
$this->checker,
48+
);
49+
$this->assertEquals(SetupResult::INFO, $check->run()->getSeverity());
50+
}
51+
52+
public function testSuccessOnEmptyResults(): void {
53+
$this->checker->expects($this->atLeastOnce())
54+
->method('isCodeCheckEnforced')
55+
->willReturn(true);
56+
$this->checker->expects($this->atLeastOnce())
57+
->method('getResults')
58+
->willReturn([]);
59+
$this->checker->expects(($this->atLeastOnce()))
60+
->method('hasPassedCheck')
61+
->willReturn(true);
62+
63+
$check = new CodeIntegrity(
64+
$this->l10n,
65+
$this->urlGenerator,
66+
$this->checker,
67+
);
68+
$this->assertEquals(SetupResult::SUCCESS, $check->run()->getSeverity());
69+
}
70+
71+
public function testCheckerIsReRunWithoutResults(): void {
72+
$this->checker->expects($this->atLeastOnce())
73+
->method('isCodeCheckEnforced')
74+
->willReturn(true);
75+
$this->checker->expects($this->atLeastOnce())
76+
->method('getResults')
77+
->willReturn(null);
78+
$this->checker->expects(($this->atLeastOnce()))
79+
->method('hasPassedCheck')
80+
->willReturn(true);
81+
82+
// This is important and must be called
83+
$this->checker->expects($this->once())
84+
->method('runInstanceVerification');
85+
86+
$check = new CodeIntegrity(
87+
$this->l10n,
88+
$this->urlGenerator,
89+
$this->checker,
90+
);
91+
$this->assertEquals(SetupResult::SUCCESS, $check->run()->getSeverity());
92+
}
93+
94+
public function testCheckerIsNotReReInAdvance(): void {
95+
$this->checker->expects($this->atLeastOnce())
96+
->method('isCodeCheckEnforced')
97+
->willReturn(true);
98+
$this->checker->expects($this->atLeastOnce())
99+
->method('getResults')
100+
->willReturn(['mocked']);
101+
$this->checker->expects(($this->atLeastOnce()))
102+
->method('hasPassedCheck')
103+
->willReturn(true);
104+
105+
// There are results thus this must never be called
106+
$this->checker->expects($this->never())
107+
->method('runInstanceVerification');
108+
109+
$check = new CodeIntegrity(
110+
$this->l10n,
111+
$this->urlGenerator,
112+
$this->checker,
113+
);
114+
$this->assertEquals(SetupResult::SUCCESS, $check->run()->getSeverity());
115+
}
116+
117+
public function testErrorOnMissingIntegrity(): void {
118+
$this->checker->expects($this->atLeastOnce())
119+
->method('isCodeCheckEnforced')
120+
->willReturn(true);
121+
$this->checker->expects($this->atLeastOnce())
122+
->method('getResults')
123+
->willReturn(['mocked']);
124+
$this->checker->expects(($this->atLeastOnce()))
125+
->method('hasPassedCheck')
126+
->willReturn(false);
127+
128+
$check = new CodeIntegrity(
129+
$this->l10n,
130+
$this->urlGenerator,
131+
$this->checker,
132+
);
133+
$this->assertEquals(SetupResult::ERROR, $check->run()->getSeverity());
134+
}
135+
}

‎lib/private/IntegrityCheck/Checker.php‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -427,26 +427,28 @@ private function verify(string $signaturePath, string $basePath, string $certifi
427427
*/
428428
public function hasPassedCheck(): bool {
429429
$results = $this->getResults();
430-
if (empty($results)) {
430+
if ($results !== null && empty($results)) {
431431
return true;
432432
}
433433

434434
return false;
435435
}
436436

437437
/**
438-
* @return array
438+
* @return array|null Either the results or null if no results available
439439
*/
440-
public function getResults(): array {
440+
public function getResults(): array|null {
441441
$cachedResults = $this->cache->get(self::CACHE_KEY);
442442
if (!\is_null($cachedResults) and $cachedResults !== false) {
443443
return json_decode($cachedResults, true);
444444
}
445445

446-
if ($this->config !== null) {
447-
return json_decode($this->config->getAppValue('core', self::CACHE_KEY, '{}'), true);
446+
$appValue = $this->config?->getAppValue('core', self::CACHE_KEY);
447+
if (!empty($appValue)) {
448+
return json_decode($appValue, true);
448449
}
449-
return [];
450+
// No results
451+
return null;
450452
}
451453

452454
/**
@@ -456,7 +458,7 @@ public function getResults(): array {
456458
* @param array $result
457459
*/
458460
private function storeResults(string $scope, array $result) {
459-
$resultArray = $this->getResults();
461+
$resultArray = $this->getResults() ?? [];
460462
unset($resultArray[$scope]);
461463
if (!empty($result)) {
462464
$resultArray[$scope] = $result;

0 commit comments

Comments
 (0)