From e30bf8148a06cb404aec8c8949ed821e7d712ef7 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sun, 13 Sep 2026 18:15:54 +0200 Subject: [PATCH] fix(encryption): use the closest cached parent for the access list Copying a folder creates the target directories on the storage before their cache entries exist, so while the files inside are written the parent of the target path can not be resolved yet. With per-user keys the encryption stream needs the access list on every write, so the unguarded $userFolder->get($parent) made every folder copy fail with a NotFoundException. Walk up to the closest ancestor that is known to the cache instead. Shares only exist on cached nodes, so its access list is the one that applies and the share keys of the recipients are still created when copying into a shared folder. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Ferdinand Thiessen --- .../files_features/encryption.feature | 59 +++++++ lib/private/Encryption/File.php | 33 +++- tests/lib/Encryption/FileTest.php | 156 ++++++++++++++++++ 3 files changed, 246 insertions(+), 2 deletions(-) create mode 100644 tests/lib/Encryption/FileTest.php diff --git a/build/integration/files_features/encryption.feature b/build/integration/files_features/encryption.feature index d961f15267151..65093dd9f4b72 100644 --- a/build/integration/files_features/encryption.feature +++ b/build/integration/files_features/encryption.feature @@ -40,3 +40,62 @@ Feature: encryption Then the command output does not contain the text "server-side encrypted: yes" And Downloading file "/non-encrypted.txt" with range "bytes=0-8" And Downloaded content should be "BLABLABLA" + + Scenario: copy a folder with per-user keys + # Setup encryption with per-user keys + Given using new dav path + And invoking occ with "app:enable encryption" + And the command was successful + And invoking occ with "encryption:disable-master-key" with input "y" + And the command was successful + And invoking occ with "encryption:enable" + And the command was successful + And user "user1" exists + And User "user1" created a folder "/source" + And User "user1" created a folder "/source/sub" + And User "user1" uploads file with content "BLABLABLA" to "/source/sub/encrypted.txt" + # The target folders only exist on the storage, not yet in the cache, while the files inside are written + When User "user1" copies file "/source" to "/copy" + Then the HTTP status code should be "201" + And As an "user1" + And Downloading file "/copy/sub/encrypted.txt" + And Downloaded content should be "BLABLABLA" + # Restore the initial encryption state + And invoking occ with "encryption:disable" + And the command was successful + And invoking occ with "encryption:enable-master-key" with input "y" + And the command was successful + + Scenario: copy a folder into a shared folder with per-user keys + # Setup encryption with per-user keys + Given using new dav path + And invoking occ with "app:enable encryption" + And the command was successful + And invoking occ with "encryption:disable-master-key" with input "y" + And the command was successful + And invoking occ with "encryption:enable" + And the command was successful + And user "user1" exists + And user "user2" exists + # Log in once so that the key pair of the share recipient exists + And User "user2" uploads file with content "BLABLABLA" to "/init.txt" + And User "user1" created a folder "/shared" + And User "user1" created a folder "/source" + And User "user1" uploads file with content "BLABLABLA" to "/source/encrypted.txt" + And as "user1" creating a share with + | path | /shared | + | shareType | 0 | + | shareWith | user2 | + | permissions | 31 | + And the HTTP status code should be "200" + # The share key of the recipient has to be created from the closest known parent + When User "user1" copies file "/source" to "/shared/copy" + Then the HTTP status code should be "201" + And As an "user2" + And Downloading file "/shared/copy/encrypted.txt" + And Downloaded content should be "BLABLABLA" + # Restore the initial encryption state + And invoking occ with "encryption:disable" + And the command was successful + And invoking occ with "encryption:enable-master-key" with input "y" + And the command was successful diff --git a/lib/private/Encryption/File.php b/lib/private/Encryption/File.php index 2c08468dffe1e..487bce719e469 100644 --- a/lib/private/Encryption/File.php +++ b/lib/private/Encryption/File.php @@ -13,6 +13,8 @@ use OCP\Cache\CappedMemoryCache; use OCP\Encryption\IFile; use OCP\Files\IRootFolder; +use OCP\Files\IUserFolder; +use OCP\Files\Node; use OCP\Files\NotFoundException; use OCP\Server; use OCP\Share\IManager; @@ -72,11 +74,14 @@ public function getAccessList($path) { // first get the shares for the parent and cache the result so that we don't // need to check all parents for every file $parent = dirname($ownerPath); - $parentNode = $userFolder->get($parent); if (isset($this->cache[$parent])) { $resultForParents = $this->cache[$parent]; } else { - $resultForParents = $this->shareManager->getAccessList($parentNode); + $resultForParents = ['users' => [], 'public' => false, 'remote' => false]; + $parentNode = $this->getClosestExistingNode($userFolder, $parent); + if ($parentNode !== null) { + $resultForParents = $this->shareManager->getAccessList($parentNode) + $resultForParents; + } $this->cache[$parent] = $resultForParents; } $userIds = array_merge($userIds, $resultForParents['users']); @@ -107,4 +112,28 @@ public function getAccessList($path) { return ['users' => $uniqueUserIds, 'public' => $public]; } + + /** + * Get the node for $path, or for its closest ancestor that is known to the cache. + * + * Copying a folder creates the target directories on the storage before their + * cache entries exist, so while the files inside are written the parent path + * can not be resolved yet. As shares only exist on nodes that are in the cache, + * the access list of the closest known ancestor also applies to $path. + * + * @return ?Node null if not even the user folder itself could be resolved + */ + private function getClosestExistingNode(IUserFolder $userFolder, string $path): ?Node { + while (true) { + try { + return $userFolder->get($path); + } catch (NotFoundException) { + $parent = dirname($path); + if ($parent === $path || $parent === '.') { + return null; + } + $path = $parent; + } + } + } } diff --git a/tests/lib/Encryption/FileTest.php b/tests/lib/Encryption/FileTest.php new file mode 100644 index 0000000000000..97598ebc36dfe --- /dev/null +++ b/tests/lib/Encryption/FileTest.php @@ -0,0 +1,156 @@ +util = $this->createMock(Util::class); + $this->shareManager = $this->createMock(IManager::class); + $this->userFolder = $this->createMock(IUserFolder::class); + + $rootFolder = $this->createMock(IRootFolder::class); + $rootFolder->method('getUserFolder') + ->with('user1') + ->willReturn($this->userFolder); + + $appManager = $this->createMock(IAppManager::class); + $appManager->method('isEnabledForUser') + ->willReturn(false); + + $this->file = $this->getMockBuilder(File::class) + ->setConstructorArgs([$this->util, $rootFolder, $this->shareManager]) + ->onlyMethods(['getAppManager']) + ->getMock(); + $this->file->method('getAppManager') + ->willReturn($appManager); + } + + /** + * Let the util mock resolve every path to $owner and $ownerPath + */ + private function mockUtil(string $owner, string $ownerPath): void { + $this->util->method('getUidAndFilename') + ->willReturn([$owner, $ownerPath]); + $this->util->method('isFile') + ->willReturn(true); + $this->util->method('stripPartialFileExtension') + ->willReturnArgument(0); + } + + public function testGetAccessList(): void { + $this->mockUtil('user1', '/files/folder/file.txt'); + + $node = $this->createMock(Node::class); + $parentNode = $this->createMock(Node::class); + $this->userFolder->method('get') + ->willReturnMap([ + ['/folder/file.txt', $node], + ['/folder', $parentNode], + ]); + + $this->shareManager->method('getAccessList') + ->willReturnCallback(fn (Node $requested) => match ($requested) { + $parentNode => ['users' => ['user2'], 'public' => false, 'remote' => false], + $node => ['users' => ['user3'], 'public' => true, 'remote' => false], + }); + + $this->assertSame( + ['users' => ['user1', 'user2', 'user3'], 'public' => true], + $this->file->getAccessList('/user1/files/folder/file.txt') + ); + } + + /** + * Copying a folder creates the target directories on the storage before their + * cache entries exist, so the parent of a file written into it can not be resolved. + */ + public function testGetAccessListWithUncachedParent(): void { + $this->mockUtil('user1', '/files/target/sub'); + + $rootNode = $this->createMock(Node::class); + $this->userFolder->method('get') + ->willReturnCallback(function (string $path) use ($rootNode) { + if ($path !== '/') { + throw new NotFoundException($path); + } + return $rootNode; + }); + + $this->shareManager->expects($this->once()) + ->method('getAccessList') + ->with($rootNode) + ->willReturn(['users' => ['user2'], 'public' => false, 'remote' => false]); + + $this->assertSame( + ['users' => ['user1', 'user2'], 'public' => false], + $this->file->getAccessList('/user1/files/target/sub') + ); + } + + public function testGetAccessListWithoutAnyResolvablePath(): void { + $this->mockUtil('user1', '/files/target/sub'); + + $this->userFolder->method('get') + ->willThrowException(new NotFoundException()); + + $this->shareManager->expects($this->never()) + ->method('getAccessList'); + + $this->assertSame( + ['users' => ['user1'], 'public' => false], + $this->file->getAccessList('/user1/files/target/sub') + ); + } + + public function testGetAccessListCachesTheParentResult(): void { + $this->util->method('getUidAndFilename') + ->willReturnCallback(fn (string $path) => ['user1', substr($path, strlen('/user1'))]); + $this->util->method('isFile') + ->willReturn(true); + $this->util->method('stripPartialFileExtension') + ->willReturnArgument(0); + + $parentNode = $this->createMock(Node::class); + $this->userFolder->method('get') + ->willReturnCallback(function (string $path) use ($parentNode) { + if ($path !== '/folder') { + throw new NotFoundException($path); + } + return $parentNode; + }); + + $this->shareManager->expects($this->once()) + ->method('getAccessList') + ->with($parentNode) + ->willReturn(['users' => ['user2'], 'public' => false, 'remote' => false]); + + $expected = ['users' => ['user1', 'user2'], 'public' => false]; + $this->assertSame($expected, $this->file->getAccessList('/user1/files/folder/first.txt')); + $this->assertSame($expected, $this->file->getAccessList('/user1/files/folder/second.txt')); + } +}