Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions changelog/unreleased/41735
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
Bugfix: Clear stale integrity check results when rescanning

The code integrity checker stores one cache entry per checked scope, but a
rescan only removed the entry holding the combined results. The per app entries
had no expiry either, so a verdict about an app was cached indefinitely and was
served even after the app had been repaired or replaced. Rescanning now clears
all of them, the entries expire, and the results are kept in the host local
cache tier - they describe the files on disk of one host and are of no use to
another.

https://github.com/owncloud/core/pull/41735
24 changes: 19 additions & 5 deletions lib/private/IntegrityCheck/Checker.php
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,14 @@
*/
class Checker implements OnDiskHasher {
public const CACHE_KEY = 'oc.integritycheck.checker';

/**
* The results describe the files on disk of this instance, so cached entries
* have to expire for a node that never runs a check itself to notice a
* repaired or replaced installation.
*/
public const CACHE_TTL = 24 * 3600;

/** @var EnvironmentHelper */
private $environmentHelper;
/** @var AppLocator */
Expand Down Expand Up @@ -104,7 +112,9 @@ public function __construct(
$this->fileAccessHelper = $fileAccessHelper;
$this->appLocator = $appLocator;
$this->config = $config;
$this->cache = $cacheFactory ? $cacheFactory->create(self::CACHE_KEY) : new \OC\Memcache\NullCache();
$this->cache = $cacheFactory
? \OC\Memcache\LocalCacheFactory::create($cacheFactory, self::CACHE_KEY)
: new \OC\Memcache\NullCache();
$this->appManager = $appManager;
$this->tempManager = $tempManager;
$this->verifier = $verifier;
Expand Down Expand Up @@ -400,17 +410,21 @@ private function storeResults($scope, array $result) {

$this->setAppValue(self::CACHE_KEY, \json_encode($resultArray));
//Set cache for each app
$this->cache->set($scope, \json_encode($resultArray));
$this->cache->set(self::CACHE_KEY, \json_encode($resultArray));
$this->cache->set($scope, \json_encode($resultArray), self::CACHE_TTL);
$this->cache->set(self::CACHE_KEY, \json_encode($resultArray), self::CACHE_TTL);
}

/**
* Clean previous results for a proper rescanning. Otherwise a stale verdict
* would be served instead of the one the rescan is about to produce.
*
* Clean previous results for a proper rescanning. Otherwise
* storeResults() writes one entry per scope in addition to CACHE_KEY, so the
* whole prefix is cleared - removing CACHE_KEY alone left every per app entry
* behind.
*/
private function cleanResults() {
$this->deleteAppValue(self::CACHE_KEY);
$this->cache->remove(self::CACHE_KEY);
$this->cache->clear();
}

/**
Expand Down
105 changes: 72 additions & 33 deletions tests/lib/IntegrityCheck/CheckerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,12 @@
use OC\IntegrityCheck\Helpers\FileAccessHelper;
use OC\IntegrityCheck\Verifier\Verifier;
use OC\IntegrityCheck\Verifier\VerificationResult;
use OC\Memcache\ArrayCache;
use OC\Memcache\NullCache;
use OC\Memcache\Redis;
use OCP\App\IAppManager;
use OCP\ICacheFactory;
use OCP\IConfig;
use Test\Memcache\FixedCacheFactory;
use Test\TestCase;

/**
Expand All @@ -48,7 +49,7 @@ class CheckerTest extends TestCase {
private $fileAccessHelper;
/** @var IConfig | \PHPUnit\Framework\MockObject\MockObject */
private $config;
/** @var ICacheFactory | \PHPUnit\Framework\MockObject\MockObject */
/** @var FixedCacheFactory */
private $cacheFactory;
/** @var IAppManager | \PHPUnit\Framework\MockObject\MockObject */
private $appManager;
Expand All @@ -61,7 +62,7 @@ public function setUp(): void {
$this->fileAccessHelper = $this->createMock(FileAccessHelper::class);
$this->appLocator = $this->createMock(AppLocator::class);
$this->config = $this->createMock(IConfig::class);
$this->cacheFactory = $this->createMock(ICacheFactory::class);
$this->cacheFactory = new FixedCacheFactory(new NullCache());
$this->appManager = $this->createMock(IAppManager::class);
$this->verifier = $this->createMock(Verifier::class);

Expand All @@ -87,12 +88,6 @@ public function setUp(): void {
->method('getAllApps')
->willReturn([]);

$this->cacheFactory
->expects($this->any())
->method('create')
->with('oc.integritycheck.checker')
->willReturn(new NullCache());

$this->checker = new Checker(
$this->environmentHelper,
$this->fileAccessHelper,
Expand All @@ -103,6 +98,69 @@ public function setUp(): void {
\OC::$server->getTempManager(),
$this->verifier
);

$this->assertSame([Checker::CACHE_KEY], $this->cacheFactory->getRequestedPrefixes());
}

/**
* The results describe the files on disk of this host, so they belong in the
* host local cache tier - not in a distributed one where another host could
* hand back a verdict about an installation it cannot see.
*/
public function testUsesTheLocalCacheTier() {
$cacheFactory = $this->createMock(FixedCacheFactory::class);
$cacheFactory->expects($this->once())
->method('createLocal')
->with(Checker::CACHE_KEY)
->willReturn(new NullCache());
$cacheFactory->expects($this->never())->method('createDistributed');
$cacheFactory->expects($this->never())->method('create');

new Checker(
$this->environmentHelper,
$this->fileAccessHelper,
$this->appLocator,
$this->config,
$cacheFactory,
$this->appManager,
\OC::$server->getTempManager(),
$this->verifier
);
}

/**
* storeResults() writes one entry per scope next to CACHE_KEY, and a rescan
* has to invalidate all of them - removing CACHE_KEY alone left every per app
* verdict cached forever.
*/
public function testRescanningDropsThePerAppResults() {
$cache = new ArrayCache();
$checker = new Checker(
$this->environmentHelper,
$this->fileAccessHelper,
$this->appLocator,
$this->config,
new FixedCacheFactory($cache),
$this->appManager,
\OC::$server->getTempManager(),
$this->verifier
);

$this->environmentHelper->method('getChannel')->willReturn('stable');
$this->environmentHelper->method('getServerRoot')->willReturn(\OC::$SERVERROOT);
$this->verifier->method('verify')->willReturn(VerificationResult::passed());

$cache->set('SomeApp', '{"SomeApp":[]}');
$cache->set('SomeOtherApp', '{"SomeOtherApp":[]}');
$cache->set(Checker::CACHE_KEY, '{"SomeApp":[]}');

$checker->runInstanceVerification();

// only the results of this run are left - the verification passed, so
// there is nothing to report
$this->assertNull($cache->get('SomeApp'));
$this->assertNull($cache->get('SomeOtherApp'));
$this->assertSame('[]', $cache->get(Checker::CACHE_KEY));
}

public function testIgnoredAppSignatureWithoutSignatureData() {
Expand Down Expand Up @@ -630,12 +688,7 @@ public function testVerifyCachedAppSignatureCheck() {
$redisObj->method('get')
->with('SomeApp')
->willReturn('[]');
$cacheFactory = $this->createMock(ICacheFactory::class);
$cacheFactory
->expects($this->any())
->method('create')
->with('oc.integritycheck.checker')
->will($this->returnValue($redisObj));
$cacheFactory = new FixedCacheFactory($redisObj);
$checker = new Checker(
$this->environmentHelper,
$this->fileAccessHelper,
Expand All @@ -654,12 +707,7 @@ public function testAppNotCachedSignatureCheck() {
$redisObj->method('get')
->with('SomeApp')
->willReturn(null);
$cacheFactory = $this->createMock(ICacheFactory::class);
$cacheFactory
->expects($this->any())
->method('create')
->with('oc.integritycheck.checker')
->will($this->returnValue($redisObj));
$cacheFactory = new FixedCacheFactory($redisObj);
$checker = new Checker(
$this->environmentHelper,
$this->fileAccessHelper,
Expand Down Expand Up @@ -702,10 +750,7 @@ public function testHasPassedCheckWithExceptionResult() {
]
]));

$cacheFactory = $this->createMock(ICacheFactory::class);
$cacheFactory->expects($this->any())
->method('create')
->willReturn(new NullCache());
$cacheFactory = new FixedCacheFactory(new NullCache());

$checker = new Checker(
$this->environmentHelper,
Expand All @@ -732,10 +777,7 @@ public function testHasPassedCheckWithEmptyResults() {
->with('core', 'oc.integritycheck.checker', '{}')
->willReturn('{}');

$cacheFactory = $this->createMock(ICacheFactory::class);
$cacheFactory->expects($this->any())
->method('create')
->willReturn(new NullCache());
$cacheFactory = new FixedCacheFactory(new NullCache());

$checker = new Checker(
$this->environmentHelper,
Expand Down Expand Up @@ -766,10 +808,7 @@ public function testHasPassedCheckWithFileMissing() {
]
]));

$cacheFactory = $this->createMock(ICacheFactory::class);
$cacheFactory->expects($this->any())
->method('create')
->willReturn(new NullCache());
$cacheFactory = new FixedCacheFactory(new NullCache());

$checker = new Checker(
$this->environmentHelper,
Expand Down