diff --git a/changelog/unreleased/41735 b/changelog/unreleased/41735 new file mode 100644 index 00000000000..599c250e4d1 --- /dev/null +++ b/changelog/unreleased/41735 @@ -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 diff --git a/lib/private/IntegrityCheck/Checker.php b/lib/private/IntegrityCheck/Checker.php index edd9a3b018c..143fe54c360 100644 --- a/lib/private/IntegrityCheck/Checker.php +++ b/lib/private/IntegrityCheck/Checker.php @@ -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 */ @@ -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; @@ -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(); } /** diff --git a/tests/lib/IntegrityCheck/CheckerTest.php b/tests/lib/IntegrityCheck/CheckerTest.php index 8cc07152f47..3a28796c8e9 100644 --- a/tests/lib/IntegrityCheck/CheckerTest.php +++ b/tests/lib/IntegrityCheck/CheckerTest.php @@ -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; /** @@ -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; @@ -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); @@ -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, @@ -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() { @@ -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, @@ -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, @@ -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, @@ -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, @@ -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,