Skip to content

Commit 8fc498f

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 <[email protected]>
1 parent acb95d5 commit 8fc498f

File tree

4 files changed

+157
-6
lines changed

4 files changed

+157
-6
lines changed

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

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

8686
$completeResults = $this->checker->getResults();
8787

88+
if ($completeResults === null) {
89+
return new DataDisplayResponse('Integrity checker has not been run. Integrity information not available.');
90+
}
91+
8892
if (!empty($completeResults)) {
8993
$formattedTextResponse = 'Technical information
9094
=====================

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,14 @@ public function getCategory(): string {
3333
public function run(): SetupResult {
3434
if (!$this->checker->isCodeCheckEnforced()) {
3535
return SetupResult::info($this->l10n->t('Integrity checker has been disabled. Integrity cannot be verified.'));
36-
} elseif ($this->checker->hasPassedCheck()) {
36+
}
37+
38+
// If there are no results we need to run the verification
39+
if ($this->checker->getResults() === null) {
40+
$this->checker->runInstanceVerification();
41+
}
42+
43+
if ($this->checker->hasPassedCheck()) {
3744
return SetupResult::success($this->l10n->t('No altered files'));
3845
} else {
3946
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: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -373,23 +373,28 @@ private function verify(string $signaturePath, string $basePath, string $certifi
373373
*/
374374
public function hasPassedCheck(): bool {
375375
$results = $this->getResults();
376-
if (empty($results)) {
376+
if ($results !== null && empty($results)) {
377377
return true;
378378
}
379379

380380
return false;
381381
}
382382

383383
/**
384-
* @return array
384+
* @return array|null Either the results or null if no results available
385385
*/
386-
public function getResults(): array {
386+
public function getResults(): array|null {
387387
$cachedResults = $this->cache->get(self::CACHE_KEY);
388388
if (!\is_null($cachedResults) and $cachedResults !== false) {
389389
return json_decode($cachedResults, true);
390390
}
391391

392-
return $this->appConfig?->getValueArray('core', self::CACHE_KEY, lazy: true) ?? [];
392+
if ($this->appConfig?->hasKey('core', self::CACHE_KEY, lazy: true)) {
393+
return $this->appConfig->getValueArray('core', self::CACHE_KEY, lazy: true);
394+
}
395+
396+
// No results available
397+
return null;
393398
}
394399

395400
/**
@@ -399,7 +404,7 @@ public function getResults(): array {
399404
* @param array $result
400405
*/
401406
private function storeResults(string $scope, array $result) {
402-
$resultArray = $this->getResults();
407+
$resultArray = $this->getResults() ?? [];
403408
unset($resultArray[$scope]);
404409
if (!empty($result)) {
405410
$resultArray[$scope] = $result;

0 commit comments

Comments
 (0)