diff --git a/changelog/unreleased/39091 b/changelog/unreleased/39091 new file mode 100644 index 000000000000..aeb82b9be5d5 --- /dev/null +++ b/changelog/unreleased/39091 @@ -0,0 +1,7 @@ +Change: Allow storage implementations to have their own encryption key storage + +The default encryption key storage location is not suitable for all storage +implementations because e.g. the storage is not linked to a user. +This is required for files_spaces. + +https://github.com/owncloud/core/pull/39091 diff --git a/lib/private/Encryption/Keys/Storage.php b/lib/private/Encryption/Keys/Storage.php index 264dd2d6cb18..97cdbf29985d 100644 --- a/lib/private/Encryption/Keys/Storage.php +++ b/lib/private/Encryption/Keys/Storage.php @@ -28,6 +28,7 @@ use OC\Files\Filesystem; use OC\Files\View; use OCP\Encryption\Keys\IStorage; +use OCP\Files\Mount\IMountManager; use OCP\IUserSession; use OC\User\NoUserException; @@ -58,13 +59,17 @@ class Storage implements IStorage { /** @var string */ private $currentUser = null; + /** + * @var IMountManager + */ + private $mountManager; /** * @param View $view view * @param Util $util encryption util class * @param IUserSession $session user session */ - public function __construct(View $view, Util $util, IUserSession $session) { + public function __construct(View $view, Util $util, IUserSession $session, IMountManager $mountManager) { $this->view = $view; $this->util = $util; @@ -75,6 +80,7 @@ public function __construct(View $view, Util $util, IUserSession $session) { if ($session !== null && $session->getUser() !== null) { $this->currentUser = $session->getUser()->getUID(); } + $this->mountManager = $mountManager; } /** @@ -274,6 +280,13 @@ private function setKey($path, $key) { * @return string */ private function getFileKeyDir($encryptionModuleId, $path) { + # ask the storage implementation for the key storage + $mount = $this->mountManager->find($path); + $keyPath = $mount ? $mount->getStorage()->getEncryptionFileKeyDirectory($encryptionModuleId, $mount->getInternalPath($path)) : null; + if ($keyPath) { + return $keyPath; + } + list($owner, $filename) = $this->util->getUidAndFilename($path); // in case of system wide mount points the keys are stored directly in the data directory diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index bcc84bc0bb88..8ffc76926736 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -797,4 +797,8 @@ public function getLocks($internalPath, $returnChildLocks = false) { return $lock; }, $locks); } + + public function getEncryptionFileKeyDirectory(string $encryptionModuleId, string $path): ?string { + return null; + } } diff --git a/lib/private/Files/Storage/Storage.php b/lib/private/Files/Storage/Storage.php index de39d00e9459..908c3458f504 100644 --- a/lib/private/Files/Storage/Storage.php +++ b/lib/private/Files/Storage/Storage.php @@ -115,4 +115,6 @@ public function releaseLock($path, $type, ILockingProvider $provider); * @throws \OCP\Lock\LockedException */ public function changeLock($path, $type, ILockingProvider $provider); + + public function getEncryptionFileKeyDirectory(string $encryptionModuleId, string $path): ?string; } diff --git a/lib/private/Files/Storage/Wrapper/Wrapper.php b/lib/private/Files/Storage/Wrapper/Wrapper.php index d7dac8be48de..42e6a907d448 100644 --- a/lib/private/Files/Storage/Wrapper/Wrapper.php +++ b/lib/private/Files/Storage/Wrapper/Wrapper.php @@ -651,4 +651,8 @@ public function getLocks($internalPath, $returnChildLocks = false) { return []; } + + public function getEncryptionFileKeyDirectory(string $encryptionModuleId, string $path): ?string { + return $this->getWrapperStorage()->getEncryptionFileKeyDirectory($encryptionModuleId, $path); + } } diff --git a/lib/private/Server.php b/lib/private/Server.php index 92e402bd1bbb..823c0eb037e1 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -216,7 +216,8 @@ public function __construct($webRoot, \OC\Config $config) { return new Encryption\Keys\Storage( $view, $util, - $c->getUserSession() + $c->getUserSession(), + $c->getMountManager() ); }); $this->registerService('TagMapper', function (Server $c) { diff --git a/tests/lib/Encryption/Keys/StorageTest.php b/tests/lib/Encryption/Keys/StorageTest.php index b6d99cb63054..2ec5a308f7e7 100644 --- a/tests/lib/Encryption/Keys/StorageTest.php +++ b/tests/lib/Encryption/Keys/StorageTest.php @@ -25,11 +25,16 @@ use OC\Encryption\Keys\Storage; use OC\Encryption\Util; +use OC\Files\Filesystem; use OC\Files\View; +use OCP\Files\Mount\IMountManager; +use OCP\Files\Mount\IMountPoint; use OCP\IUser; use OCP\IUserSession; +use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; use Test\Traits\UserTrait; +use OCP\IConfig; /** * Class StorageTest @@ -44,56 +49,66 @@ class StorageTest extends TestCase { /** @var Storage */ protected $storage; - /** @var \PHPUnit\Framework\MockObject\MockObject | Util */ + /** @var MockObject | Util */ protected $util; - /** @var \PHPUnit\Framework\MockObject\MockObject | View */ + /** @var MockObject | View */ protected $view; - /** @var \PHPUnit\Framework\MockObject\MockObject */ + /** @var MockObject */ protected $config; + /** + * @var string[] + */ + private $mkdirStack; + /** + * @var IMountManager|MockObject + */ + private $mountManager; public function setUp(): void { parent::setUp(); - $this->util = $this->getMockBuilder('OC\Encryption\Util') + $this->util = $this->getMockBuilder(Util::class) ->disableOriginalConstructor() ->getMock(); $this->util->method('getKeyStorageRoot')->willReturn(''); - $this->view = $this->getMockBuilder('OC\Files\View') + $this->view = $this->getMockBuilder(View::class) ->disableOriginalConstructor() ->getMock(); - $this->config = $this->getMockBuilder('OCP\IConfig') + $this->config = $this->getMockBuilder(IConfig::class) ->disableOriginalConstructor() ->getMock(); + $this->mountManager = $this->createMock(IMountManager::class); + $user = $this->createMock(IUser::class); $user->method('getUID')->willReturn('user1'); - /** @var IUserSession | \PHPUnit\Framework\MockObject\MockObject $userSession */ + /** @var IUserSession | MockObject $userSession */ $userSession = $this->createMock(IUserSession::class); $userSession->method('getUser')->willReturn($user); $this->createUser('user1', '123456'); $this->createUser('user2', '123456'); - $this->storage = new Storage($this->view, $this->util, $userSession); + $this->storage = new Storage($this->view, $this->util, $userSession, $this->mountManager); } public function tearDown(): void { - \OC\Files\Filesystem::tearDown(); + Filesystem::tearDown(); parent::tearDown(); } - public function testSetFileKey() { - $this->util->expects($this->any()) + public function testSetFileKey(): void { + $this->util ->method('getUidAndFilename') ->willReturn(['user1', '/files/foo.txt']); - $this->util->expects($this->any()) + $this->util ->method('stripPartialFileExtension') ->willReturnArgument(0); - $this->util->expects($this->any()) + $this->util ->method('isSystemWideMountPoint') ->willReturn(false); $this->view->expects($this->once()) @@ -109,7 +124,7 @@ public function testSetFileKey() { ); } - public function dataTestGetFileKey() { + public function dataTestGetFileKey(): array { return [ ['/files/foo.txt', '/files/foo.txt', true, 'key'], ['/files/foo.txt.ocTransferId2111130212.part', '/files/foo.txt', true, 'key'], @@ -125,8 +140,8 @@ public function dataTestGetFileKey() { * @param bool $originalKeyExists * @param string $expectedKeyContent */ - public function testGetFileKey($path, $strippedPartialName, $originalKeyExists, $expectedKeyContent) { - $this->util->expects($this->any()) + public function testGetFileKey($path, $strippedPartialName, $originalKeyExists, $expectedKeyContent): void { + $this->util ->method('getUidAndFilename') ->willReturnMap([ ['user1/files/foo.txt', ['user1', '/files/foo.txt']], @@ -137,7 +152,7 @@ public function testGetFileKey($path, $strippedPartialName, $originalKeyExists, $this->util->expects($this->once()) ->method('stripPartialFileExtension') ->willReturn('user1' . $strippedPartialName); - $this->util->expects($this->any()) + $this->util ->method('isSystemWideMountPoint') ->willReturn(false); @@ -176,14 +191,14 @@ public function testGetFileKey($path, $strippedPartialName, $originalKeyExists, ); } - public function testSetFileKeySystemWide() { - $this->util->expects($this->any()) + public function testSetFileKeySystemWide(): void { + $this->util ->method('getUidAndFilename') ->willReturn(['user1', '/files/foo.txt']); - $this->util->expects($this->any()) + $this->util ->method('isSystemWideMountPoint') ->willReturn(true); - $this->util->expects($this->any()) + $this->util ->method('stripPartialFileExtension') ->willReturnArgument(0); $this->view->expects($this->once()) @@ -199,14 +214,14 @@ public function testSetFileKeySystemWide() { ); } - public function testGetFileKeySystemWide() { - $this->util->expects($this->any()) + public function testGetFileKeySystemWide(): void { + $this->util ->method('getUidAndFilename') ->willReturn(['user1', '/files/foo.txt']); - $this->util->expects($this->any()) + $this->util ->method('stripPartialFileExtension') ->willReturnArgument(0); - $this->util->expects($this->any()) + $this->util ->method('isSystemWideMountPoint') ->willReturn(true); $this->view->expects($this->once()) @@ -224,7 +239,7 @@ public function testGetFileKeySystemWide() { ); } - public function testSetSystemUserKey() { + public function testSetSystemUserKey(): void { $this->view->expects($this->once()) ->method('file_put_contents') ->with( @@ -238,7 +253,7 @@ public function testSetSystemUserKey() { ); } - public function testSetUserKey() { + public function testSetUserKey(): void { $this->view->expects($this->once()) ->method('file_put_contents') ->with( @@ -252,7 +267,7 @@ public function testSetUserKey() { ); } - public function testGetSystemUserKey() { + public function testGetSystemUserKey(): void { $this->view->expects($this->once()) ->method('file_get_contents') ->with($this->equalTo('/files_encryption/encModule/shareKey_56884')) @@ -268,7 +283,7 @@ public function testGetSystemUserKey() { ); } - public function testGetUserKey() { + public function testGetUserKey(): void { $this->view->expects($this->once()) ->method('file_get_contents') ->with($this->equalTo('/user1/files_encryption/encModule/user1.publicKey')) @@ -284,7 +299,7 @@ public function testGetUserKey() { ); } - public function testGetUserKeyShared() { + public function testGetUserKeyShared(): void { $this->view->expects($this->once()) ->method('file_get_contents') ->with($this->equalTo('/user2/files_encryption/encModule/user2.publicKey')) @@ -304,7 +319,7 @@ public function testGetUserKeyShared() { $this->assertTrue($this->isUserHomeMounted('user2')); } - public function testGetUserKeyWhenKeyStorageIsOutsideHome() { + public function testGetUserKeyWhenKeyStorageIsOutsideHome(): void { $this->view->expects($this->once()) ->method('file_get_contents') ->with($this->equalTo('/enckeys/user2/files_encryption/encModule/user2.publicKey')) @@ -318,9 +333,9 @@ public function testGetUserKeyWhenKeyStorageIsOutsideHome() { $user->method('getUID')->willReturn('user1'); $userSession = $this->createMock(IUserSession::class); $userSession->method('getUser')->willReturn($user); - $util = $this->createMock(\OC\Encryption\Util::class); + $util = $this->createMock(Util::class); $util->method('getKeyStorageRoot')->willReturn('enckeys'); - $storage = new Storage($this->view, $util, $userSession); + $storage = new Storage($this->view, $util, $userSession, $this->mountManager); $this->assertFalse($this->isUserHomeMounted('user2')); @@ -338,7 +353,7 @@ public function testGetUserKeyWhenKeyStorageIsOutsideHome() { * @param string $userId * @return boolean true if mounted, false otherwise */ - private function isUserHomeMounted($userId) { + private function isUserHomeMounted($userId): bool { // verify that user2's FS got mounted when retrieving the key $mountManager = \OC::$server->getMountManager(); $mounts = $mountManager->getAll(); @@ -349,7 +364,7 @@ private function isUserHomeMounted($userId) { return !empty($mounts); } - public function testDeleteUserKey() { + public function testDeleteUserKey(): void { $this->view->expects($this->once()) ->method('file_exists') ->with($this->equalTo('/user1/files_encryption/encModule/user1.publicKey')) @@ -364,7 +379,7 @@ public function testDeleteUserKey() { ); } - public function testDeleteSystemUserKey() { + public function testDeleteSystemUserKey(): void { $this->view->expects($this->once()) ->method('file_exists') ->with($this->equalTo('/files_encryption/encModule/shareKey_56884')) @@ -379,14 +394,14 @@ public function testDeleteSystemUserKey() { ); } - public function testDeleteFileKeySystemWide() { - $this->util->expects($this->any()) + public function testDeleteFileKeySystemWide(): void { + $this->util ->method('getUidAndFilename') ->willReturn(['user1', '/files/foo.txt']); - $this->util->expects($this->any()) + $this->util ->method('stripPartialFileExtension') ->willReturnArgument(0); - $this->util->expects($this->any()) + $this->util ->method('isSystemWideMountPoint') ->willReturn(true); $this->view->expects($this->once()) @@ -403,14 +418,14 @@ public function testDeleteFileKeySystemWide() { ); } - public function testDeleteFileKey() { - $this->util->expects($this->any()) + public function testDeleteFileKey(): void { + $this->util ->method('getUidAndFilename') ->willReturn(['user1', '/files/foo.txt']); - $this->util->expects($this->any()) + $this->util ->method('stripPartialFileExtension') ->willReturnArgument(0); - $this->util->expects($this->any()) + $this->util ->method('isSystemWideMountPoint') ->willReturn(false); $this->view->expects($this->once()) @@ -430,11 +445,11 @@ public function testDeleteFileKey() { /** * @dataProvider dataProviderCopyRename */ - public function testRenameKeys($source, $target, $systemWideMountSource, $systemWideMountTarget, $expectedSource, $expectedTarget) { - $this->view->expects($this->any()) + public function testRenameKeys($source, $target, $systemWideMountSource, $systemWideMountTarget, $expectedSource, $expectedTarget): void { + $this->view ->method('file_exists') ->willReturn(true); - $this->view->expects($this->any()) + $this->view ->method('is_dir') ->willReturn(true); $this->view->expects($this->once()) @@ -444,12 +459,12 @@ public function testRenameKeys($source, $target, $systemWideMountSource, $system $this->equalTo($expectedTarget) ) ->willReturn(true); - $this->util->expects($this->any()) + $this->util ->method('getUidAndFilename') - ->will($this->returnCallback([$this, 'getUidAndFilenameCallback'])); - $this->util->expects($this->any()) + ->willReturnCallback([$this, 'getUidAndFilenameCallback']); + $this->util ->method('isSystemWideMountPoint') - ->willReturnCallback(function ($path, $owner) use ($systemWideMountSource, $systemWideMountTarget) { + ->willReturnCallback(function ($path) use ($systemWideMountSource, $systemWideMountTarget) { if (\strpos($path, 'source.txt') !== false) { return $systemWideMountSource; } @@ -462,11 +477,11 @@ public function testRenameKeys($source, $target, $systemWideMountSource, $system /** * @dataProvider dataProviderCopyRename */ - public function testCopyKeys($source, $target, $systemWideMountSource, $systemWideMountTarget, $expectedSource, $expectedTarget) { - $this->view->expects($this->any()) + public function testCopyKeys($source, $target, $systemWideMountSource, $systemWideMountTarget, $expectedSource, $expectedTarget): void { + $this->view ->method('file_exists') ->willReturn(true); - $this->view->expects($this->any()) + $this->view ->method('is_dir') ->willReturn(true); $this->view->expects($this->once()) @@ -476,12 +491,12 @@ public function testCopyKeys($source, $target, $systemWideMountSource, $systemWi $this->equalTo($expectedTarget) ) ->willReturn(true); - $this->util->expects($this->any()) + $this->util ->method('getUidAndFilename') - ->will($this->returnCallback([$this, 'getUidAndFilenameCallback'])); - $this->util->expects($this->any()) + ->willReturnCallback([$this, 'getUidAndFilenameCallback']); + $this->util ->method('isSystemWideMountPoint') - ->willReturnCallback(function ($path, $owner) use ($systemWideMountSource, $systemWideMountTarget) { + ->willReturnCallback(function ($path) use ($systemWideMountSource, $systemWideMountTarget) { if (\strpos($path, 'source.txt') !== false) { return $systemWideMountSource; } @@ -491,7 +506,7 @@ public function testCopyKeys($source, $target, $systemWideMountSource, $systemWi $this->storage->copyKeys($source, $target); } - public function getUidAndFilenameCallback() { + public function getUidAndFilenameCallback(): array { $args = \func_get_args(); $path = $args[0]; @@ -500,7 +515,7 @@ public function getUidAndFilenameCallback() { return [$parts[1], '/' . \implode('/', \array_slice($parts, 2))]; } - public function dataProviderCopyRename() { + public function dataProviderCopyRename(): array { return [ ['/user1/files/source.txt', '/user1/files/target.txt', false, false, '/user1/files_encryption/keys/files/source.txt/', '/user1/files_encryption/keys/files/target.txt/'], @@ -538,13 +553,13 @@ public function dataProviderCopyRename() { * @param string $storageRoot * @param string $expected */ - public function testGetPathToKeys($path, $systemWideMountPoint, $storageRoot, $expected) { - $this->invokePrivate($this->storage, 'root_dir', [$storageRoot]); + public function testGetPathToKeys($path, $systemWideMountPoint, $storageRoot, $expected): void { + self::invokePrivate($this->storage, 'root_dir', [$storageRoot]); - $this->util->expects($this->any()) + $this->util ->method('getUidAndFilename') - ->will($this->returnCallback([$this, 'getUidAndFilenameCallback'])); - $this->util->expects($this->any()) + ->willReturnCallback([$this, 'getUidAndFilenameCallback']); + $this->util ->method('isSystemWideMountPoint') ->willReturn($systemWideMountPoint); @@ -554,7 +569,7 @@ public function testGetPathToKeys($path, $systemWideMountPoint, $storageRoot, $e ); } - public function dataTestGetPathToKeys() { + public function dataTestGetPathToKeys(): array { return [ ['/user1/files/source.txt', false, '', '/user1/files_encryption/keys/files/source.txt/'], ['/user1/files/source.txt', true, '', '/files_encryption/keys/files/source.txt/'], @@ -563,16 +578,16 @@ public function dataTestGetPathToKeys() { ]; } - public function testKeySetPreparation() { - $this->view->expects($this->any()) + public function testKeySetPreparation(): void { + $this->view ->method('file_exists') ->willReturn(false); - $this->view->expects($this->any()) + $this->view ->method('is_dir') ->willReturn(false); - $this->view->expects($this->any()) + $this->view ->method('mkdir') - ->will($this->returnCallback([$this, 'mkdirCallback'])); + ->willReturnCallback([$this, 'mkdirCallback']); $this->mkdirStack = [ '/user1/files_encryption/keys/foo', @@ -583,7 +598,7 @@ public function testKeySetPreparation() { self::invokePrivate($this->storage, 'keySetPreparation', ['/user1/files_encryption/keys/foo']); } - public function mkdirCallback() { + public function mkdirCallback(): void { $args = \func_get_args(); $expected = \array_pop($this->mkdirStack); $this->assertSame($expected, $args[0]); @@ -591,35 +606,44 @@ public function mkdirCallback() { /** * @dataProvider dataTestGetFileKeyDir - * - * @param bool $isSystemWideMountPoint - * @param string $storageRoot - * @param string $expected */ - public function testGetFileKeyDir($isSystemWideMountPoint, $storageRoot, $expected) { + public function testGetFileKeyDir($isSystemWideMountPoint, $storageRoot, $expected, $hasMountPoint = false, $storagePath = null, $expectDefaultImpl = true): void { $path = '/user1/files/foo/bar.txt'; $owner = 'user1'; $relativePath = '/foo/bar.txt'; - $this->invokePrivate($this->storage, 'root_dir', [$storageRoot]); + if ($hasMountPoint) { + $storage = $this->createMock(\OC\Files\Storage\Storage::class); + $storage->method('getEncryptionFileKeyDirectory') + ->with('OC_DEFAULT_MODULE', $relativePath) + ->willReturn($storagePath); + $mount = $this->createMock(IMountPoint::class); + $mount->method('getStorage')->willReturn($storage); + $mount->method('getInternalPath')->willReturn($relativePath); + $this->mountManager->method('find')->willReturn($mount); + } + + self::invokePrivate($this->storage, 'root_dir', [$storageRoot]); - $this->util->expects($this->once())->method('isSystemWideMountPoint') + $this->util->expects($expectDefaultImpl ? $this->once(): $this->never())->method('isSystemWideMountPoint') ->willReturn($isSystemWideMountPoint); - $this->util->expects($this->once())->method('getUidAndFilename') + $this->util->expects($expectDefaultImpl ? $this->once(): $this->never())->method('getUidAndFilename') ->with($path)->willReturn([$owner, $relativePath]); $this->assertSame( $expected, - $this->invokePrivate($this->storage, 'getFileKeyDir', ['OC_DEFAULT_MODULE', $path]) + self::invokePrivate($this->storage, 'getFileKeyDir', ['OC_DEFAULT_MODULE', $path]) ); } - public function dataTestGetFileKeyDir() { + public function dataTestGetFileKeyDir(): array { return [ [false, '', '/user1/files_encryption/keys/foo/bar.txt/OC_DEFAULT_MODULE/'], [true, '', '/files_encryption/keys/foo/bar.txt/OC_DEFAULT_MODULE/'], [false, 'newStorageRoot', '/newStorageRoot/user1/files_encryption/keys/foo/bar.txt/OC_DEFAULT_MODULE/'], [true, 'newStorageRoot', '/newStorageRoot/files_encryption/keys/foo/bar.txt/OC_DEFAULT_MODULE/'], + [false, '', '/user1/files_encryption/keys/foo/bar.txt/OC_DEFAULT_MODULE/', true, null], + [false, '', '/foo/bar/keys/foo/bar.txt/OC_DEFAULT_MODULE/', true, '/foo/bar/keys/foo/bar.txt/OC_DEFAULT_MODULE/', false], ]; } }