diff --git a/apps/encryption/tests/EncryptedCopyTest.php b/apps/encryption/tests/EncryptedCopyTest.php new file mode 100644 index 0000000000000..6aa44509af277 --- /dev/null +++ b/apps/encryption/tests/EncryptedCopyTest.php @@ -0,0 +1,139 @@ +validateMasterKey(); + Server::get(KeyManager::class)->validateShareKey(); + $this->createUser('test1', 'test2'); + $this->setupForUser('test1', 'test2'); + $this->registerMount('test1', new Temporary(), '/test1/files/other'); + $this->loginWithEncryption('test1'); + + return new View('/test1/files'); + } + + /** + * The version the target ends up at depends on the storage: a copy unlinks the + * target first, which keeps the cache entry - and with it the version to bump - + * on a local storage but drops it on an object store, where the copy therefore + * starts over at version 1. Reading the target back is what shows that the + * recorded version is the one its blocks were signed with. + */ + public function testCopyOverExistingFile(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->file_put_contents('source.bin', $source); + $view->file_put_contents('target.bin', str_repeat('b', 20000)); + + $this->assertTrue($view->copy('source.bin', 'target.bin')); + + $this->assertEquals($source, $view->file_get_contents('target.bin')); + } + + /** + * Every write bumps the version of the source, while the copy of it starts over + * at the version of the target. + */ + public function testCopyFileWrittenSeveralTimes(): void { + $view = $this->setUpView(); + $source = str_repeat('c', 20000); + + $view->file_put_contents('source.bin', str_repeat('a', 20000)); + $view->file_put_contents('source.bin', str_repeat('b', 20000)); + $view->file_put_contents('source.bin', $source); + + $this->assertTrue($view->copy('source.bin', 'target.bin')); + + $this->assertEquals($source, $view->file_get_contents('target.bin')); + } + + public function testCopyOverExistingFileWrittenSeveralTimes(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->file_put_contents('source.bin', $source); + $view->file_put_contents('target.bin', str_repeat('b', 20000)); + $view->file_put_contents('target.bin', str_repeat('c', 20000)); + + $this->assertTrue($view->copy('source.bin', 'target.bin')); + + $this->assertEquals($source, $view->file_get_contents('target.bin')); + } + + public function testCopyFolderOverExistingFolder(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->mkdir('source'); + $view->file_put_contents('source/file.bin', $source); + $view->mkdir('target'); + $view->file_put_contents('target/file.bin', str_repeat('b', 20000)); + + $this->assertTrue($view->copy('source', 'target')); + + $this->assertEquals($source, $view->file_get_contents('target/file.bin')); + } + + public function testMoveOverExistingFileOnAnotherStorage(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->file_put_contents('source.bin', $source); + $view->file_put_contents('other/target.bin', str_repeat('b', 20000)); + + $this->assertTrue($view->rename('source.bin', 'other/target.bin')); + + $this->assertEquals($source, $view->file_get_contents('other/target.bin')); + } + + /** + * The target is not unlinked when it lives on another storage, so it keeps its + * cache entry and the copy is signed with the version that follows the one of + * the file it overwrites. + */ + public function testCopyOverExistingFileOnAnotherStorage(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->file_put_contents('source.bin', $source); + $view->file_put_contents('other/target.bin', str_repeat('b', 20000)); + + $this->assertTrue($view->copy('source.bin', 'other/target.bin')); + + $this->assertEquals($source, $view->file_get_contents('other/target.bin')); + $this->assertEquals( + 2, + $view->getFileInfo('other/target.bin')->getEncryptedVersion(), + 'the version of the overwritten file was not bumped' + ); + } +} diff --git a/build/integration/encryption_features/encryption.feature b/build/integration/encryption_features/encryption.feature index 15704609d79df..d64d429e72ce9 100644 --- a/build/integration/encryption_features/encryption.feature +++ b/build/integration/encryption_features/encryption.feature @@ -29,6 +29,28 @@ Feature: encryption When Downloading file "/copy.bin" Then the HTTP status code should be "200" + Scenario: Copy a file over an existing file + Given user "user0" exists + And As an "user0" + And User "user0" uploads file with content "the source content" to "/source.txt" + And User "user0" uploads file with content "the target content" to "/target.txt" + When User "user0" copies file "/source.txt" to "/target.txt" + Then the HTTP status code should be "204" + When Downloading file "/target.txt" + Then the HTTP status code should be "200" + And Downloaded content should be "the source content" + + Scenario: Copy a file that was written several times + Given user "user0" exists + And As an "user0" + And User "user0" uploads file with content "the first content" to "/source.txt" + And User "user0" uploads file with content "the second content" to "/source.txt" + When User "user0" copies file "/source.txt" to "/copy.txt" + Then the HTTP status code should be "201" + When Downloading file "/copy.txt" + Then the HTTP status code should be "200" + And Downloaded content should be "the second content" + # With "part_file_in_storage" disabled the part file is written to the user # home while the target lives on another storage, so the upload has to read the # part file back to move it over. A part file never has a file cache entry, so diff --git a/lib/private/Files/Cache/Cache.php b/lib/private/Files/Cache/Cache.php index 7c8023c90c1d6..014bf6ed6381e 100644 --- a/lib/private/Files/Cache/Cache.php +++ b/lib/private/Files/Cache/Cache.php @@ -1282,6 +1282,14 @@ public function copyFromCache(ICache $sourceCache, ICacheEntry $sourceEntry, str // normalizeData() prefers 'encryptedVersion' over 'encrypted' when both are // set, so it has to be cleared too or the mark above gets ignored unset($data['encryptedVersion']); + } elseif (isset($data['encryptedVersion'])) { + // The storage re-encrypts the content it writes to the target, so the target + // is at its own version - the one recorded for it while it was written - and + // not at the version of the source. + $targetEntry = $this->get($targetPath); + if ($targetEntry !== false && !empty($targetEntry['encryptedVersion'])) { + $data['encryptedVersion'] = $targetEntry['encryptedVersion']; + } } $fileId = $this->put($targetPath, $data); diff --git a/lib/private/Files/Storage/Wrapper/Encryption.php b/lib/private/Files/Storage/Wrapper/Encryption.php index 66b1c7aa78611..421d75cee7613 100644 --- a/lib/private/Files/Storage/Wrapper/Encryption.php +++ b/lib/private/Files/Storage/Wrapper/Encryption.php @@ -604,13 +604,26 @@ private function updateEncryptedVersion( // Rename of the cache already happened, so we do the cleanup on the target if ($sourceCacheEntry === false && $targetCacheEntry !== false) { - $encryptedVersion = $targetCacheEntry['encryptedVersion']; $isRename = false; - } elseif ($sourceCacheEntry === false) { - // a file that is not in the file cache, e.g. a part file, is at version 1 - $encryptedVersion = 1; + } + + if ($keepEncryptionVersion) { + // a 1:1 copy reuses the keys and the ciphertext of the source, so the + // target stays at the version of the source + if ($sourceCacheEntry !== false) { + $encryptedVersion = (int)($sourceCacheEntry['encryptedVersion'] ?? 0); + } elseif ($targetCacheEntry !== false) { + $encryptedVersion = (int)($targetCacheEntry['encryptedVersion'] ?? 0); + } else { + // a file that is not in the file cache, e.g. a part file, is at version 1 + $encryptedVersion = 1; + } } else { - $encryptedVersion = $sourceCacheEntry['encryptedVersion']; + // The target was written through the encryption stream, which signs the + // blocks with the version that follows the version of the file they + // replaced and records it on the target's cache entry. A target that has + // no cache entry was written at version 1. + $encryptedVersion = $targetCacheEntry === false ? 1 : (int)($targetCacheEntry['encryptedVersion'] ?? 0); } // In case of a move operation from an unencrypted to an encrypted @@ -618,7 +631,7 @@ private function updateEncryptedVersion( // correct value would be "1". Thus we manually set the value to "1" // for those cases. // See also https://github.com/owncloud/core/issues/23078 - if ($encryptedVersion === 0 || !$keepEncryptionVersion) { + if ($encryptedVersion === 0) { $encryptedVersion = 1; } diff --git a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php index 6bb960fb3e306..4e97e1374e6ba 100644 --- a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php +++ b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php @@ -723,6 +723,75 @@ public static function dataCopyBetweenStorage(): array { ]; } + public static function dataUpdateEncryptedVersion(): array { + return [ + // the target is written through the encryption stream, which signs its blocks + // with the version that follows the version of the file they replace + 'copy onto an existing file' => [['encryptedVersion' => 4], ['encryptedVersion' => 3], false, 3], + 'copy onto a new file' => [['encryptedVersion' => 4], false, false, 1], + 'copy onto a file that is not encrypted yet' => [['encryptedVersion' => 4], ['encryptedVersion' => 0], false, 1], + // a 1:1 copy reuses the keys and the ciphertext of the source + '1:1 copy' => [['encryptedVersion' => 5], false, true, 5], + '1:1 copy of a file that is not encrypted yet' => [['encryptedVersion' => 0], false, true, 1], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('dataUpdateEncryptedVersion')] + public function testUpdateEncryptedVersion( + array|false $sourceCacheEntry, + array|false $targetCacheEntry, + bool $keepEncryptionVersion, + int $expectedVersion, + ): void { + $sourceCache = $this->createMock(ICache::class); + $sourceCache->method('get') + ->with('source.txt') + ->willReturn($sourceCacheEntry); + $sourceStorage = $this->createMock(\OC\Files\Storage\Storage::class); + $sourceStorage->method('getCache') + ->willReturn($sourceCache); + + $targetCache = $this->createMock(ICache::class); + $targetCache->method('get') + ->with('target.txt') + ->willReturn($targetCacheEntry); + $targetCache->expects($this->once()) + ->method('put') + ->with('target.txt', ['encrypted' => true, 'encryptedVersion' => $expectedVersion]); + + $instance = $this->getMockBuilder(Encryption::class) + ->setConstructorArgs( + [ + [ + 'storage' => $this->sourceStorage, + 'root' => 'foo', + 'mountPoint' => '/', + 'mount' => $this->mount + ], + $this->encryptionManager, + $this->util, + $this->logger, + $this->file, + null, + $this->keyStore, + $this->mountManager, + $this->arrayCache + ] + ) + ->onlyMethods(['getCache', 'getEncryptionModule']) + ->getMock(); + $instance->method('getCache')->willReturn($targetCache); + $instance->method('getEncryptionModule')->willReturn($this->encryptionModule); + + $this->encryptionManager->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); + global $mockedMountPointEncryptionEnabled; + $mockedMountPointEncryptionEnabled = true; + + $this->invokePrivate($instance, 'updateEncryptedVersion', [$sourceStorage, 'source.txt', 'target.txt', false, $keepEncryptionVersion]); + } + public function testCopyBetweenStorageMinimumEncryptedVersion(): void { $storage2 = $this->createMock(\OC\Files\Storage\Storage::class);