From 399e5545ebc05d6d97b03950786821d13550d794 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 13 Dec 2016 19:10:51 +0100 Subject: [PATCH 1/8] The file system of a user has to be properly setup before accessing the keys --- lib/private/Encryption/Keys/Storage.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/private/Encryption/Keys/Storage.php b/lib/private/Encryption/Keys/Storage.php index f977cd4aab2e..dbfaf566bce0 100644 --- a/lib/private/Encryption/Keys/Storage.php +++ b/lib/private/Encryption/Keys/Storage.php @@ -70,6 +70,10 @@ public function __construct(View $view, Util $util) { * @inheritdoc */ public function getUserKey($uid, $keyId, $encryptionModuleId) { + $currentUser = \OC_User::getUser(); + if (!is_null($uid) && $uid !== '' && $uid !== $currentUser) { + \OC\Files\Filesystem::initMountPoints($uid); + } $path = $this->constructUserKeyPath($encryptionModuleId, $keyId, $uid); return $this->getKey($path); } From f0643f1187b4622ec254bf38f789149c7e4be03b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Wed, 14 Dec 2016 21:41:06 +0100 Subject: [PATCH 2/8] Fix unit test execution --- tests/lib/Encryption/Keys/StorageTest.php | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/tests/lib/Encryption/Keys/StorageTest.php b/tests/lib/Encryption/Keys/StorageTest.php index b5b91f886a38..482d01a2f6c1 100644 --- a/tests/lib/Encryption/Keys/StorageTest.php +++ b/tests/lib/Encryption/Keys/StorageTest.php @@ -24,17 +24,29 @@ namespace Test\Encryption\Keys; use OC\Encryption\Keys\Storage; +use OC\Encryption\Util; +use OC\Files\View; use Test\TestCase; +use Test\Traits\UserTrait; +/** + * Class StorageTest + * + * @group DB + * + * @package Test\Encryption\Keys + */ class StorageTest extends TestCase { + use UserTrait; + /** @var Storage */ protected $storage; - /** @var \PHPUnit_Framework_MockObject_MockObject */ + /** @var \PHPUnit_Framework_MockObject_MockObject | Util */ protected $util; - /** @var \PHPUnit_Framework_MockObject_MockObject */ + /** @var \PHPUnit_Framework_MockObject_MockObject | View */ protected $view; /** @var \PHPUnit_Framework_MockObject_MockObject */ @@ -232,6 +244,7 @@ public function testGetUserKey() { ->with($this->equalTo('/user1/files_encryption/encModule/user1.publicKey')) ->willReturn(true); + $this->createUser('user1', '123456'); $this->assertSame('key', $this->storage->getUserKey('user1', 'publicKey', 'encModule') ); From 3899b812c349ad632d0d898d25cac76b2ecf2cf0 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 20 Dec 2016 10:22:56 +0100 Subject: [PATCH 3/8] Init mount points for user in more places in Keys\Storage --- lib/private/Encryption/Keys/Storage.php | 30 +++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/lib/private/Encryption/Keys/Storage.php b/lib/private/Encryption/Keys/Storage.php index dbfaf566bce0..7dc6123b5456 100644 --- a/lib/private/Encryption/Keys/Storage.php +++ b/lib/private/Encryption/Keys/Storage.php @@ -53,6 +53,9 @@ class Storage implements IStorage { /** @var array */ private $keyCache = []; + /** @var string */ + private $currentUser = null; + /** * @param View $view * @param Util $util @@ -64,16 +67,17 @@ public function __construct(View $view, Util $util) { $this->encryption_base_dir = '/files_encryption'; $this->keys_base_dir = $this->encryption_base_dir .'/keys'; $this->root_dir = $this->util->getKeyStorageRoot(); + + $session = \OC::$server->getUserSession(); + if (!is_null($session) && !is_null($session->getUser())) { + $this->currentUser = $session->getUser()->getUID(); + } } /** * @inheritdoc */ public function getUserKey($uid, $keyId, $encryptionModuleId) { - $currentUser = \OC_User::getUser(); - if (!is_null($uid) && $uid !== '' && $uid !== $currentUser) { - \OC\Files\Filesystem::initMountPoints($uid); - } $path = $this->constructUserKeyPath($encryptionModuleId, $keyId, $uid); return $this->getKey($path); } @@ -174,6 +178,7 @@ protected function constructUserKeyPath($encryptionModuleId, $keyId, $uid) { if ($uid === null) { $path = $this->root_dir . '/' . $this->encryption_base_dir . '/' . $encryptionModuleId . '/' . $keyId; } else { + $this->setupUserMounts($uid); $path = $this->root_dir . '/' . $uid . $this->encryption_base_dir . '/' . $encryptionModuleId . '/' . $uid . '.' . $keyId; } @@ -239,6 +244,7 @@ private function getFileKeyDir($encryptionModuleId, $path) { if ($this->util->isSystemWideMountPoint($filename, $owner)) { $keyPath = $this->root_dir . '/' . $this->keys_base_dir . $filename . '/'; } else { + $this->setupUserMounts($owner); $keyPath = $this->root_dir . '/' . $owner . $this->keys_base_dir . $filename . '/'; } @@ -302,6 +308,7 @@ protected function getPathToKeys($path) { if ($systemWideMountPoint) { $systemPath = $this->root_dir . '/' . $this->keys_base_dir . $relativePath . '/'; } else { + $this->setupUserMounts($owner); $systemPath = $this->root_dir . '/' . $owner . $this->keys_base_dir . $relativePath . '/'; } @@ -327,4 +334,19 @@ protected function keySetPreparation($path) { } } + /** + * Setup the mounts of the given user if different than + * the current user. + * + * This is needed because in many cases the keys are stored + * within the user's home storage. + * + * @param string $uid user id + */ + protected function setupUserMounts($uid) { + if (!is_null($uid) && $uid !== '' && $uid !== $this->currentUser) { + \OC\Files\Filesystem::initMountPoints($uid); + } + } + } From b93253a9892b6cba8d605aadb4e138dd6e3b6eca Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 10 Jan 2017 19:38:20 +0100 Subject: [PATCH 4/8] Fix encryption key storage tests to properly create required users --- tests/lib/Encryption/Keys/StorageTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/lib/Encryption/Keys/StorageTest.php b/tests/lib/Encryption/Keys/StorageTest.php index 482d01a2f6c1..cb09e261206b 100644 --- a/tests/lib/Encryption/Keys/StorageTest.php +++ b/tests/lib/Encryption/Keys/StorageTest.php @@ -67,6 +67,8 @@ public function setUp() { ->disableOriginalConstructor() ->getMock(); + $this->createUser('user1', '123456'); + $this->createUser('user2', '123456'); $this->storage = new Storage($this->view, $this->util); } @@ -244,7 +246,6 @@ public function testGetUserKey() { ->with($this->equalTo('/user1/files_encryption/encModule/user1.publicKey')) ->willReturn(true); - $this->createUser('user1', '123456'); $this->assertSame('key', $this->storage->getUserKey('user1', 'publicKey', 'encModule') ); From e421d69b88e349a4c8756236bba0f0f0c0ed5f64 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 12 Jan 2017 15:50:24 +0100 Subject: [PATCH 5/8] Added test that check if user home is mounted after resolving key --- lib/private/Encryption/Keys/Storage.php | 9 +++-- lib/private/Server.php | 6 ++- tests/lib/Encryption/Keys/StorageTest.php | 45 ++++++++++++++++++++++- 3 files changed, 54 insertions(+), 6 deletions(-) diff --git a/lib/private/Encryption/Keys/Storage.php b/lib/private/Encryption/Keys/Storage.php index 7dc6123b5456..5d31757b681f 100644 --- a/lib/private/Encryption/Keys/Storage.php +++ b/lib/private/Encryption/Keys/Storage.php @@ -27,6 +27,7 @@ use OC\Files\Filesystem; use OC\Files\View; use OCP\Encryption\Keys\IStorage; +use OCP\IUserSession; class Storage implements IStorage { @@ -57,10 +58,11 @@ class Storage implements IStorage { private $currentUser = null; /** - * @param View $view - * @param Util $util + * @param View $view view + * @param Util $util encryption util class + * @param IUserSession $session user session */ - public function __construct(View $view, Util $util) { + public function __construct(View $view, Util $util, IUserSession $session) { $this->view = $view; $this->util = $util; @@ -68,7 +70,6 @@ public function __construct(View $view, Util $util) { $this->keys_base_dir = $this->encryption_base_dir .'/keys'; $this->root_dir = $this->util->getKeyStorageRoot(); - $session = \OC::$server->getUserSession(); if (!is_null($session) && !is_null($session->getUser())) { $this->currentUser = $session->getUser()->getUID(); } diff --git a/lib/private/Server.php b/lib/private/Server.php index 8345a0b66e09..9430ebe800bc 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -148,7 +148,11 @@ public function __construct($webRoot, \OC\Config $config) { $c->getConfig() ); - return new Encryption\Keys\Storage($view, $util); + return new Encryption\Keys\Storage( + $view, + $util, + $c->getUserSession() + ); }); $this->registerService('TagMapper', function (Server $c) { return new TagMapper($c->getDatabaseConnection()); diff --git a/tests/lib/Encryption/Keys/StorageTest.php b/tests/lib/Encryption/Keys/StorageTest.php index cb09e261206b..13b23aa75b4a 100644 --- a/tests/lib/Encryption/Keys/StorageTest.php +++ b/tests/lib/Encryption/Keys/StorageTest.php @@ -28,6 +28,8 @@ use OC\Files\View; use Test\TestCase; use Test\Traits\UserTrait; +use OCP\IUser; +use OCP\IUserSession; /** * Class StorageTest @@ -67,9 +69,14 @@ public function setUp() { ->disableOriginalConstructor() ->getMock(); + $user = $this->getMock('\OCP\IUser'); + $user->method('getUID')->willReturn('user1'); + $userSession = $this->getMock('\OCP\IUserSession'); + $userSession->method('getUser')->willReturn($user); + $this->createUser('user1', '123456'); $this->createUser('user2', '123456'); - $this->storage = new Storage($this->view, $this->util); + $this->storage = new Storage($this->view, $this->util, $userSession); } public function testSetFileKey() { @@ -251,6 +258,42 @@ public function testGetUserKey() { ); } + public function testGetUserKeyShared() { + $this->view->expects($this->once()) + ->method('file_get_contents') + ->with($this->equalTo('/user2/files_encryption/encModule/user2.publicKey')) + ->willReturn('key'); + $this->view->expects($this->once()) + ->method('file_exists') + ->with($this->equalTo('/user2/files_encryption/encModule/user2.publicKey')) + ->willReturn(true); + + $this->assertFalse($this->isUserHomeMounted('user2')); + + $this->assertSame('key', + $this->storage->getUserKey('user2', 'publicKey', 'encModule') + ); + + $this->assertTrue($this->isUserHomeMounted('user2')); + } + + /** + * Returns whether the home of the given user was mounted + * + * @param string $userId + * @return boolean true if mounted, false otherwise + */ + private function isUserHomeMounted($userId) { + // verify that user2's FS got mounted when retrieving the key + $mountManager = \OC::$server->getMountManager(); + $mounts = $mountManager->getAll(); + $mounts = array_filter($mounts, function($mount) use ($userId) { + return ($mount->getMountPoint() === "/$userId/"); + }); + + return !empty($mounts); + } + public function testDeleteUserKey() { $this->view->expects($this->once()) ->method('file_exists') From cc29f883c018d843650b38376ecfafd238db930e Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Fri, 13 Jan 2017 11:51:32 +0100 Subject: [PATCH 6/8] Don't mount user home with alternate keys root When encryption keys are stored outside the user's homes, there is no need to mount said homes. --- lib/private/Encryption/Keys/Storage.php | 5 ++++ tests/lib/Encryption/Keys/StorageTest.php | 33 +++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/lib/private/Encryption/Keys/Storage.php b/lib/private/Encryption/Keys/Storage.php index 5d31757b681f..98d1fe25e425 100644 --- a/lib/private/Encryption/Keys/Storage.php +++ b/lib/private/Encryption/Keys/Storage.php @@ -345,6 +345,11 @@ protected function keySetPreparation($path) { * @param string $uid user id */ protected function setupUserMounts($uid) { + if ($this->root_dir !== '') { + // this means that the keys are stored outside of the user's homes, + // so we don't need to mount anything + return; + } if (!is_null($uid) && $uid !== '' && $uid !== $this->currentUser) { \OC\Files\Filesystem::initMountPoints($uid); } diff --git a/tests/lib/Encryption/Keys/StorageTest.php b/tests/lib/Encryption/Keys/StorageTest.php index 13b23aa75b4a..e9a5144dd64d 100644 --- a/tests/lib/Encryption/Keys/StorageTest.php +++ b/tests/lib/Encryption/Keys/StorageTest.php @@ -61,6 +61,8 @@ public function setUp() { ->disableOriginalConstructor() ->getMock(); + $this->util->method('getKeyStorageRoot')->willReturn(''); + $this->view = $this->getMockBuilder('OC\Files\View') ->disableOriginalConstructor() ->getMock(); @@ -79,6 +81,10 @@ public function setUp() { $this->storage = new Storage($this->view, $this->util, $userSession); } + public function tearDown() { + \OC\Files\Filesystem::tearDown(); + } + public function testSetFileKey() { $this->util->expects($this->any()) ->method('getUidAndFilename') @@ -277,6 +283,33 @@ public function testGetUserKeyShared() { $this->assertTrue($this->isUserHomeMounted('user2')); } + public function testGetUserKeyWhenKeyStorageIsOutsideHome() { + $this->view->expects($this->once()) + ->method('file_get_contents') + ->with($this->equalTo('/enckeys/user2/files_encryption/encModule/user2.publicKey')) + ->willReturn('key'); + $this->view->expects($this->once()) + ->method('file_exists') + ->with($this->equalTo('/enckeys/user2/files_encryption/encModule/user2.publicKey')) + ->willReturn(true); + + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user1'); + $userSession = $this->createMock(IUserSession::class); + $userSession->method('getUser')->willReturn($user); + $util = $this->createMock(\OC\Encryption\Util::class); + $util->method('getKeyStorageRoot')->willReturn('enckeys'); + $storage = new Storage($this->view, $util, $userSession); + + $this->assertFalse($this->isUserHomeMounted('user2')); + + $this->assertSame('key', + $storage->getUserKey('user2', 'publicKey', 'encModule') + ); + + $this->assertFalse($this->isUserHomeMounted('user2'), 'mounting was not necessary'); + } + /** * Returns whether the home of the given user was mounted * From 9ccc59b0c1665ab46033cf68522a58d0b9d13715 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 18 Jan 2017 17:42:07 +0100 Subject: [PATCH 7/8] Ignore exception when deleting keys of deleted user Whenever a user was deleted for encryption where the keys are stored in the home, we can ignore user existence exceptions because it means the keys are already gone. --- lib/private/Encryption/Keys/Storage.php | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/lib/private/Encryption/Keys/Storage.php b/lib/private/Encryption/Keys/Storage.php index 98d1fe25e425..e9364b5ba582 100644 --- a/lib/private/Encryption/Keys/Storage.php +++ b/lib/private/Encryption/Keys/Storage.php @@ -28,6 +28,7 @@ use OC\Files\View; use OCP\Encryption\Keys\IStorage; use OCP\IUserSession; +use OC\User\NoUserException; class Storage implements IStorage { @@ -138,8 +139,21 @@ public function setSystemUserKey($keyId, $key, $encryptionModuleId) { * @inheritdoc */ public function deleteUserKey($uid, $keyId, $encryptionModuleId) { - $path = $this->constructUserKeyPath($encryptionModuleId, $keyId, $uid); - return !$this->view->file_exists($path) || $this->view->unlink($path); + try { + $path = $this->constructUserKeyPath($encryptionModuleId, $keyId, $uid); + return !$this->view->file_exists($path) || $this->view->unlink($path); + } catch (NoUserException $e) { + // this exception can come from initMountPoints() from setupUserMounts() + // for a deleted user. + // + // It means, that: + // - we are not running in alternative storage mode because we don't call + // initMountPoints() in that mode + // - the keys were in the user's home but since the user was deleted, the + // user's home is gone and so are the keys + // + // So there is nothing to do, just ignore. + } } /** From 9c1574a74bbb71ee4c5238c32ed6921ba5fc7f2d Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 19 Jan 2017 16:16:00 +0100 Subject: [PATCH 8/8] Fix encryption key storage test for older PHP --- tests/lib/Encryption/Keys/StorageTest.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/lib/Encryption/Keys/StorageTest.php b/tests/lib/Encryption/Keys/StorageTest.php index e9a5144dd64d..d4271e7570ae 100644 --- a/tests/lib/Encryption/Keys/StorageTest.php +++ b/tests/lib/Encryption/Keys/StorageTest.php @@ -28,8 +28,6 @@ use OC\Files\View; use Test\TestCase; use Test\Traits\UserTrait; -use OCP\IUser; -use OCP\IUserSession; /** * Class StorageTest @@ -293,11 +291,13 @@ public function testGetUserKeyWhenKeyStorageIsOutsideHome() { ->with($this->equalTo('/enckeys/user2/files_encryption/encModule/user2.publicKey')) ->willReturn(true); - $user = $this->createMock(IUser::class); + $user = $this->getMock('\OCP\IUser'); $user->method('getUID')->willReturn('user1'); - $userSession = $this->createMock(IUserSession::class); + $userSession = $this->getMock('\OCP\IUserSession'); $userSession->method('getUser')->willReturn($user); - $util = $this->createMock(\OC\Encryption\Util::class); + $util = $this->getMockBuilder('\OC\Encryption\Util') + ->disableOriginalConstructor() + ->getMock(); $util->method('getKeyStorageRoot')->willReturn('enckeys'); $storage = new Storage($this->view, $util, $userSession);