From e0dada704cebb0a61f6fd8e845f0d8865a373672 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 18 Mar 2014 19:37:02 +0100 Subject: [PATCH 01/10] Added ext storage unit tests for writing then reload the mount config --- apps/files_external/tests/mountconfig.php | 74 ++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-) diff --git a/apps/files_external/tests/mountconfig.php b/apps/files_external/tests/mountconfig.php index a22c7424c6..090b5f8e5c 100644 --- a/apps/files_external/tests/mountconfig.php +++ b/apps/files_external/tests/mountconfig.php @@ -113,7 +113,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { * Test adding a global mount point */ public function testAddGlobalMountPoint() { - $mountType = OC_Mount_Config::MOUNT_TYPE_GLOBAL; + $mountType = OC_Mount_Config::MOUNT_TYPE_USER; $applicable = 'all'; $isPersonal = false; @@ -186,4 +186,76 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { $this->assertFalse(OC_Mount_Config::addMountPoint('/ext', $storageClass, array(), $mountType, $applicable, $isPersonal)); } + + /** + * Test reading and writing global config + */ + public function testReadWriteGlobalConfig() { + $mountType = OC_Mount_Config::MOUNT_TYPE_USER; + $applicable = 'all'; + $isPersonal = false; + $mountConfig = array( + 'host' => 'smbhost', + 'user' => 'smbuser', + 'password' => 'smbpassword', + 'share' => 'smbshare', + 'root' => 'smbroot' + ); + + // write config + $this->assertTrue( + OC_Mount_Config::addMountPoint( + '/ext', + '\OC\Files\Storage\SMB', + $mountConfig, + $mountType, + $applicable, + $isPersonal + ) + ); + + // re-read config + $config = OC_Mount_Config::getSystemMountPoints(); + $this->assertEquals(1, count($config)); + $this->assertTrue(isset($config['ext'])); + $this->assertEquals('\OC\Files\Storage\SMB', $config['ext']['class']); + $savedMountConfig = $config['ext']['configuration']; + $this->assertEquals($mountConfig, $savedMountConfig); + } + + /** + * Test reading and writing config + */ + public function testReadWritePersonalConfig() { + $mountType = OC_Mount_Config::MOUNT_TYPE_USER; + $applicable = 'test'; + $isPersonal = true; + $mountConfig = array( + 'host' => 'smbhost', + 'user' => 'smbuser', + 'password' => 'smbpassword', + 'share' => 'smbshare', + 'root' => 'smbroot' + ); + + // write config + $this->assertTrue( + OC_Mount_Config::addMountPoint( + '/ext', + '\OC\Files\Storage\SMB', + $mountConfig, + $mountType, + $applicable, + $isPersonal + ) + ); + + // re-read config + $config = OC_Mount_Config::getPersonalMountPoints(); + $this->assertEquals(1, count($config)); + $this->assertTrue(isset($config['ext'])); + $this->assertEquals('\OC\Files\Storage\SMB', $config['ext']['class']); + $savedMountConfig = $config['ext']['configuration']; + $this->assertEquals($mountConfig, $savedMountConfig); + } } From 40a70ecf7906ce9ee631d237b4af6c21f5930a0f Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 18 Mar 2014 21:15:11 +0100 Subject: [PATCH 02/10] Added password obfuscation for external storage config Added obfuscation for all "password" options from external storages. Added unit tests for reading/writing the configuration. --- apps/files_external/lib/config.php | 62 +++++++++++++++- apps/files_external/lib/smb.php | 2 +- apps/files_external/tests/mountconfig.php | 87 +++++++++++++++++++++++ 3 files changed, 148 insertions(+), 3 deletions(-) diff --git a/apps/files_external/lib/config.php b/apps/files_external/lib/config.php index 2c8828c4d5..05cc88c5d5 100755 --- a/apps/files_external/lib/config.php +++ b/apps/files_external/lib/config.php @@ -4,6 +4,7 @@ * * @author Michael Gapczynski * @copyright 2012 Michael Gapczynski mtgap@owncloud.com +* @copyright 2014 Vincent Petry * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU AFFERO GENERAL PUBLIC LICENSE @@ -19,9 +20,16 @@ * License along with this library. If not, see . */ +set_include_path( + get_include_path() . PATH_SEPARATOR . + \OC_App::getAppPath('files_external') . '/3rdparty/phpseclib/phpseclib' +); +include('Crypt/AES.php'); + /** * Class to configure the config/mount.php and data/$user/mount.php files -*/ + */ +// TODO: make this class non-static class OC_Mount_Config { const MOUNT_TYPE_GLOBAL = 'global'; @@ -31,6 +39,9 @@ class OC_Mount_Config { // whether to skip backend test (for unit tests, as this static class is not mockable) public static $skipTest = false; + // password encryption cipher + private static $cipher; + /** * Get details on each of the external storage backends, used for the mount config UI * If a custom UI is needed, add the key 'custom' and a javascript file with that name will be loaded @@ -203,6 +214,7 @@ class OC_Mount_Config { if (strpos($mount['class'], 'OC_Filestorage_') !== false) { $mount['class'] = '\OC\Files\Storage\\'.substr($mount['class'], 15); } + $mount['options'] = self::decryptPasswords($mount['options']); // Remove '/$user/files/' from mount point $mountPoint = substr($mountPoint, 13); // Merge the mount point into the current mount points @@ -228,6 +240,7 @@ class OC_Mount_Config { if (strpos($mount['class'], 'OC_Filestorage_') !== false) { $mount['class'] = '\OC\Files\Storage\\'.substr($mount['class'], 15); } + $mount['options'] = self::decryptPasswords($mount['options']); // Remove '/$user/files/' from mount point $mountPoint = substr($mountPoint, 13); // Merge the mount point into the current mount points @@ -265,6 +278,7 @@ class OC_Mount_Config { if (strpos($mount['class'], 'OC_Filestorage_') !== false) { $mount['class'] = '\OC\Files\Storage\\'.substr($mount['class'], 15); } + $mount['options'] = self::decryptPasswords($mount['options']); // Remove '/uid/files/' from mount point $personal[substr($mountPoint, strlen($uid) + 8)] = array( 'class' => $mount['class'], @@ -334,7 +348,13 @@ class OC_Mount_Config { } else { $mountPoint = '/$user/files/'.ltrim($mountPoint, '/'); } - $mount = array($applicable => array($mountPoint => array('class' => $class, 'options' => $classOptions))); + + $mount = array($applicable => array( + $mountPoint => array( + 'class' => $class, + 'options' => self::encryptPasswords($classOptions)) + ) + ); $mountPoints = self::readData($isPersonal); // Merge the new mount point into the current mount points if (isset($mountPoints[$mountType])) { @@ -527,4 +547,42 @@ class OC_Mount_Config { return $txt; } + + /** + * Encrypt passwords in the given config options + * @param array $options mount options + * @return array updated options + */ + private static function encryptPasswords($options) { + if (isset($options['password'])) { + $options['password_encrypted'] = base64_encode(self::getCipher()->encrypt($options['password'])); + unset($options['password']); + } + return $options; + } + + /** + * Decrypt passwords in the given config options + * @param array $options mount options + * @return array updated options + */ + private static function decryptPasswords($options) { + // note: legacy options might still have the unencrypted password in the "password" field + if (isset($options['password_encrypted'])) { + $options['password'] = self::getCipher()->decrypt(base64_decode($options['password_encrypted'])); + unset($options['password_encrypted']); + } + return $options; + } + + /** + * Returns the encryption cipher + */ + private static function getCipher() { + if (!isset(self::$cipher)) { + self::$cipher = new Crypt_AES(CRYPT_AES_MODE_CBC); + self::$cipher->setKey(\OCP\Config::getSystemValue('passwordsalt')); + } + return self::$cipher; + } } diff --git a/apps/files_external/lib/smb.php b/apps/files_external/lib/smb.php index c5fba92ee6..f3f3b3ed7f 100644 --- a/apps/files_external/lib/smb.php +++ b/apps/files_external/lib/smb.php @@ -37,7 +37,7 @@ class SMB extends \OC\Files\Storage\StreamWrapper{ $this->share = substr($this->share, 0, -1); } } else { - throw new \Exception(); + throw new \Exception('Invalid configuration'); } } diff --git a/apps/files_external/tests/mountconfig.php b/apps/files_external/tests/mountconfig.php index 090b5f8e5c..8c255769fc 100644 --- a/apps/files_external/tests/mountconfig.php +++ b/apps/files_external/tests/mountconfig.php @@ -94,6 +94,14 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { return json_decode(file_get_contents($configFile), true); } + /** + * Write the user config, to simulate existing files + */ + private function writeUserConfig($config) { + $configFile = $this->userHome . '/mount.json'; + file_put_contents($configFile, json_encode($config)); + } + /** * Test mount point validation */ @@ -258,4 +266,83 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { $savedMountConfig = $config['ext']['configuration']; $this->assertEquals($mountConfig, $savedMountConfig); } + + /** + * Test password obfuscation + */ + public function testPasswordObfuscation() { + $mountType = OC_Mount_Config::MOUNT_TYPE_USER; + $applicable = 'test'; + $isPersonal = true; + $mountConfig = array( + 'host' => 'smbhost', + 'user' => 'smbuser', + 'password' => 'smbpassword', + 'share' => 'smbshare', + 'root' => 'smbroot' + ); + + // write config + $this->assertTrue( + OC_Mount_Config::addMountPoint( + '/ext', + '\OC\Files\Storage\SMB', + $mountConfig, + $mountType, + $applicable, + $isPersonal + ) + ); + + // note: password re-reading is covered by testReadWritePersonalConfig + + // check that password inside the file is NOT in plain text + $config = $this->readUserConfig(); + $savedConfig = $config[$mountType][$applicable]['/test/files/ext']['options']; + + // no more clear text password in file + $this->assertFalse(isset($savedConfig['password'])); + + // encrypted password is present + $this->assertNotEquals($mountConfig['password'], $savedConfig['password_encrypted']); + } + + /** + * Test read legacy passwords + */ + public function testReadLegacyPassword() { + $mountType = OC_Mount_Config::MOUNT_TYPE_USER; + $applicable = 'test'; + $isPersonal = true; + $mountConfig = array( + 'host' => 'smbhost', + 'user' => 'smbuser', + 'password' => 'smbpassword', + 'share' => 'smbshare', + 'root' => 'smbroot' + ); + + // write config + $this->assertTrue( + OC_Mount_Config::addMountPoint( + '/ext', + '\OC\Files\Storage\SMB', + $mountConfig, + $mountType, + $applicable, + $isPersonal + ) + ); + + $config = $this->readUserConfig(); + // simulate non-encrypted password situation + $config[$mountType][$applicable]['/test/files/ext']['options']['password'] = 'smbpasswd'; + + $this->writeUserConfig($config); + + // re-read config, password was read correctly + $config = OC_Mount_Config::getPersonalMountPoints(); + $savedMountConfig = $config['ext']['configuration']; + $this->assertEquals($mountConfig, $savedMountConfig); + } } From 80180bea325c994be3f24d14ec7dd0682429396d Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 19 Mar 2014 11:42:22 +0100 Subject: [PATCH 03/10] Added IV for ext storage password encryption --- apps/files_external/lib/config.php | 43 +++++++++++++++++++++++------- lib/public/util.php | 9 +++++++ 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/apps/files_external/lib/config.php b/apps/files_external/lib/config.php index 05cc88c5d5..0a68de1cdb 100755 --- a/apps/files_external/lib/config.php +++ b/apps/files_external/lib/config.php @@ -39,9 +39,6 @@ class OC_Mount_Config { // whether to skip backend test (for unit tests, as this static class is not mockable) public static $skipTest = false; - // password encryption cipher - private static $cipher; - /** * Get details on each of the external storage backends, used for the mount config UI * If a custom UI is needed, add the key 'custom' and a javascript file with that name will be loaded @@ -555,7 +552,7 @@ class OC_Mount_Config { */ private static function encryptPasswords($options) { if (isset($options['password'])) { - $options['password_encrypted'] = base64_encode(self::getCipher()->encrypt($options['password'])); + $options['password_encrypted'] = self::encryptPassword($options['password']); unset($options['password']); } return $options; @@ -569,20 +566,46 @@ class OC_Mount_Config { private static function decryptPasswords($options) { // note: legacy options might still have the unencrypted password in the "password" field if (isset($options['password_encrypted'])) { - $options['password'] = self::getCipher()->decrypt(base64_decode($options['password_encrypted'])); + $options['password'] = self::decryptPassword($options['password_encrypted']); unset($options['password_encrypted']); } return $options; } + /** + * Encrypt a single password + * @param string $password plain text password + * @return encrypted password + */ + private static function encryptPassword($password) { + $cipher = self::getCipher(); + $iv = \OCP\Util::generateRandomBytes(16); + $cipher->setIV($iv); + return base64_encode($iv . $cipher->encrypt($password)); + } + + /** + * Decrypts a single password + * @param string $encryptedPassword encrypted password + * @return plain text password + */ + private static function decryptPassword($encryptedPassword) { + $cipher = self::getCipher(); + $binaryPassword = base64_decode($encryptedPassword); + $iv = substr($binaryPassword, 0, 16); + $cipher->setIV($iv); + $binaryPassword = substr($binaryPassword, 16); + return $cipher->decrypt($binaryPassword); + } + /** * Returns the encryption cipher */ private static function getCipher() { - if (!isset(self::$cipher)) { - self::$cipher = new Crypt_AES(CRYPT_AES_MODE_CBC); - self::$cipher->setKey(\OCP\Config::getSystemValue('passwordsalt')); - } - return self::$cipher; + // note: not caching this to make it thread safe as we'll use + // a different IV for each password + $cipher = new Crypt_AES(CRYPT_AES_MODE_CBC); + $cipher->setKey(\OCP\Config::getSystemValue('passwordsalt')); + return $cipher; } } diff --git a/lib/public/util.php b/lib/public/util.php index 585c5d2263..f4d749c104 100644 --- a/lib/public/util.php +++ b/lib/public/util.php @@ -495,4 +495,13 @@ class Util { public static function isValidFileName($file) { return \OC_Util::isValidFileName($file); } + + /** + * @brief Generates a cryptographic secure pseudo-random string + * @param Int $length of the random string + * @return String + */ + public static function generateRandomBytes($length = 30) { + return \OC_Util::generateRandomBytes($length); + } } From 4cb53f77b2e09fa16129700b459e8b4edcd3eb64 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 19 Mar 2014 12:20:48 +0100 Subject: [PATCH 04/10] Moved external storage mounting code to files_external app Moved the mounting code for external storage from OC\Filesystem::initMountPoint to files_external using the post_initMountPoints hook --- apps/files_external/appinfo/app.php | 3 +- apps/files_external/lib/config.php | 87 ++++++++++++++++++++++++++++- lib/private/files/filesystem.php | 70 ----------------------- 3 files changed, 86 insertions(+), 74 deletions(-) diff --git a/apps/files_external/appinfo/app.php b/apps/files_external/appinfo/app.php index 5b1cd86a17..0e83660f84 100644 --- a/apps/files_external/appinfo/app.php +++ b/apps/files_external/appinfo/app.php @@ -25,5 +25,6 @@ if (OCP\Config::getAppValue('files_external', 'allow_user_mounting', 'yes') == ' } // connecting hooks -OCP\Util::connectHook( 'OC_User', 'post_login', 'OC\Files\Storage\iRODS', 'login' ); +OCP\Util::connectHook('OC_Filesystem', 'post_initMountPoints', '\OC_Mount_Config', 'initMountPointsHook'); +OCP\Util::connectHook('OC_User', 'post_login', 'OC\Files\Storage\iRODS', 'login'); diff --git a/apps/files_external/lib/config.php b/apps/files_external/lib/config.php index 0a68de1cdb..28761b4862 100755 --- a/apps/files_external/lib/config.php +++ b/apps/files_external/lib/config.php @@ -27,10 +27,10 @@ set_include_path( include('Crypt/AES.php'); /** -* Class to configure the config/mount.php and data/$user/mount.php files + * Class to configure mount.json globally and for users */ -// TODO: make this class non-static class OC_Mount_Config { + // TODO: make this class non-static and give it a proper namespace const MOUNT_TYPE_GLOBAL = 'global'; const MOUNT_TYPE_GROUP = 'group'; @@ -166,6 +166,81 @@ class OC_Mount_Config { return($backends); } + /** + * Init mount points hook + * @param array $data + */ + public static function initMountPointsHook($data) { + $user = $data['user']; + $root = $data['user_dir']; + + $datadir = \OC_Config::getValue("datadirectory", \OC::$SERVERROOT . "/data"); + $mount_file = \OC_Config::getValue("mount_file", $datadir . "/mount.json"); + + //move config file to it's new position + if (is_file(\OC::$SERVERROOT . '/config/mount.json')) { + rename(\OC::$SERVERROOT . '/config/mount.json', $mount_file); + } + + // Load system mount points + $mountConfig = self::readData(false); + if (isset($mountConfig[self::MOUNT_TYPE_GLOBAL])) { + foreach ($mountConfig[self::MOUNT_TYPE_GLOBAL] as $mountPoint => $options) { + $options['options'] = self::decryptPasswords($options['options']); + \OC\Files\Filesystem::mount($options['class'], $options['options'], $mountPoint); + } + } + if (isset($mountConfig[self::MOUNT_TYPE_GROUP])) { + foreach ($mountConfig[self::MOUNT_TYPE_GROUP] as $group => $mounts) { + if (\OC_Group::inGroup($user, $group)) { + foreach ($mounts as $mountPoint => $options) { + $mountPoint = self::setUserVars($user, $mountPoint); + foreach ($options as &$option) { + $option = self::setUserVars($user, $option); + } + $options['options'] = self::decryptPasswords($options['options']); + \OC\Files\Filesystem::mount($options['class'], $options['options'], $mountPoint); + } + } + } + } + if (isset($mountConfig[self::MOUNT_TYPE_USER])) { + foreach ($mountConfig[self::MOUNT_TYPE_USER] as $mountUser => $mounts) { + if ($mountUser === 'all' or strtolower($mountUser) === strtolower($user)) { + foreach ($mounts as $mountPoint => $options) { + $mountPoint = self::setUserVars($user, $mountPoint); + foreach ($options as &$option) { + $option = self::setUserVars($user, $option); + } + $options['options'] = self::decryptPasswords($options['options']); + \OC\Files\Filesystem::mount($options['class'], $options['options'], $mountPoint); + } + } + } + } + + // Load personal mount points + $mountConfig = self::readData(true); + if (isset($mountConfig[self::MOUNT_TYPE_USER][$user])) { + foreach ($mountConfig[self::MOUNT_TYPE_USER][$user] as $mountPoint => $options) { + $options['options'] = self::decryptPasswords($options['options']); + \OC\Files\Filesystem::mount($options['class'], $options['options'], $mountPoint); + } + } + } + + /** + * fill in the correct values for $user + * + * @param string $user + * @param string $input + * @return string + */ + private static function setUserVars($user, $input) { + return str_replace('$user', $user, $input); + } + + /** * Get details on each of the external storage backends, used for the mount config UI * Some backends are not available as a personal backend, f.e. Local and such that have @@ -288,12 +363,18 @@ class OC_Mount_Config { return $personal; } + /** + * Test connecting using the given backend configuration + * @param string $class backend class name + * @param array $options backend configuration options + * @return bool true if the connection succeeded, false otherwise + */ private static function getBackendStatus($class, $options) { if (self::$skipTest) { return true; } foreach ($options as &$option) { - $option = str_replace('$user', OCP\User::getUser(), $option); + $option = self::setUserVars(OCP\User::getUser(), $option); } if (class_exists($class)) { try { diff --git a/lib/private/files/filesystem.php b/lib/private/files/filesystem.php index 6478854eae..c31e0c3818 100644 --- a/lib/private/files/filesystem.php +++ b/lib/private/files/filesystem.php @@ -320,81 +320,11 @@ class Filesystem { else { self::mount('\OC\Files\Storage\Local', array('datadir' => $root), $user); } - $datadir = \OC_Config::getValue("datadirectory", \OC::$SERVERROOT . "/data"); - $mount_file = \OC_Config::getValue("mount_file", $datadir . "/mount.json"); - - //move config file to it's new position - if (is_file(\OC::$SERVERROOT . '/config/mount.json')) { - rename(\OC::$SERVERROOT . '/config/mount.json', $mount_file); - } - // Load system mount points - if (is_file(\OC::$SERVERROOT . '/config/mount.php') or is_file($mount_file)) { - if (is_file($mount_file)) { - $mountConfig = json_decode(file_get_contents($mount_file), true); - } elseif (is_file(\OC::$SERVERROOT . '/config/mount.php')) { - $mountConfig = $parser->parsePHP(file_get_contents(\OC::$SERVERROOT . '/config/mount.php')); - } - if (isset($mountConfig['global'])) { - foreach ($mountConfig['global'] as $mountPoint => $options) { - self::mount($options['class'], $options['options'], $mountPoint); - } - } - if (isset($mountConfig['group'])) { - foreach ($mountConfig['group'] as $group => $mounts) { - if (\OC_Group::inGroup($user, $group)) { - foreach ($mounts as $mountPoint => $options) { - $mountPoint = self::setUserVars($user, $mountPoint); - foreach ($options as &$option) { - $option = self::setUserVars($user, $option); - } - self::mount($options['class'], $options['options'], $mountPoint); - } - } - } - } - if (isset($mountConfig['user'])) { - foreach ($mountConfig['user'] as $mountUser => $mounts) { - if ($mountUser === 'all' or strtolower($mountUser) === strtolower($user)) { - foreach ($mounts as $mountPoint => $options) { - $mountPoint = self::setUserVars($user, $mountPoint); - foreach ($options as &$option) { - $option = self::setUserVars($user, $option); - } - self::mount($options['class'], $options['options'], $mountPoint); - } - } - } - } - } - // Load personal mount points - if (is_file($root . '/mount.php') or is_file($root . '/mount.json')) { - if (is_file($root . '/mount.json')) { - $mountConfig = json_decode(file_get_contents($root . '/mount.json'), true); - } elseif (is_file($root . '/mount.php')) { - $mountConfig = $parser->parsePHP(file_get_contents($root . '/mount.php')); - } - if (isset($mountConfig['user'][$user])) { - foreach ($mountConfig['user'][$user] as $mountPoint => $options) { - self::mount($options['class'], $options['options'], $mountPoint); - } - } - } // Chance to mount for other storages \OC_Hook::emit('OC_Filesystem', 'post_initMountPoints', array('user' => $user, 'user_dir' => $root)); } - /** - * fill in the correct values for $user - * - * @param string $user - * @param string $input - * @return string - */ - private static function setUserVars($user, $input) { - return str_replace('$user', $user, $input); - } - /** * get the default filesystem view * From 8e0a5ed5df4dbea5bacb5b03640cb989d97a9629 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 19 Mar 2014 14:23:36 +0100 Subject: [PATCH 05/10] Added tests to check mount point list for a target user --- apps/files_external/lib/config.php | 29 ++++-- apps/files_external/tests/mountconfig.php | 110 ++++++++++++++++++++++ 2 files changed, 132 insertions(+), 7 deletions(-) diff --git a/apps/files_external/lib/config.php b/apps/files_external/lib/config.php index 28761b4862..6de7c91358 100755 --- a/apps/files_external/lib/config.php +++ b/apps/files_external/lib/config.php @@ -167,12 +167,25 @@ class OC_Mount_Config { } /** - * Init mount points hook + * Hook that mounts the given user's visible mount points * @param array $data */ public static function initMountPointsHook($data) { - $user = $data['user']; - $root = $data['user_dir']; + $mountPoints = self::getAbsoluteMountPoints($data['user']); + foreach ($mountPoints as $mountPoints => $options) { + \OC\Files\Filesystem::mount($options['class'], $options['options'], $mountPoint); + } + } + + /** + * Returns the mount points for the given user. + * The mount point is relative to the data directory. + * + * @param string $user user + * @return array of mount point string as key, mountpoint config as value + */ + public static function getAbsoluteMountPoints($user) { + $mountPoints = array(); $datadir = \OC_Config::getValue("datadirectory", \OC::$SERVERROOT . "/data"); $mount_file = \OC_Config::getValue("mount_file", $datadir . "/mount.json"); @@ -187,7 +200,7 @@ class OC_Mount_Config { if (isset($mountConfig[self::MOUNT_TYPE_GLOBAL])) { foreach ($mountConfig[self::MOUNT_TYPE_GLOBAL] as $mountPoint => $options) { $options['options'] = self::decryptPasswords($options['options']); - \OC\Files\Filesystem::mount($options['class'], $options['options'], $mountPoint); + $mountPoints[$mountPoint] = $options; } } if (isset($mountConfig[self::MOUNT_TYPE_GROUP])) { @@ -199,7 +212,7 @@ class OC_Mount_Config { $option = self::setUserVars($user, $option); } $options['options'] = self::decryptPasswords($options['options']); - \OC\Files\Filesystem::mount($options['class'], $options['options'], $mountPoint); + $mountPoints[$mountPoint] = $options; } } } @@ -213,7 +226,7 @@ class OC_Mount_Config { $option = self::setUserVars($user, $option); } $options['options'] = self::decryptPasswords($options['options']); - \OC\Files\Filesystem::mount($options['class'], $options['options'], $mountPoint); + $mountPoints[$mountPoint] = $options; } } } @@ -224,9 +237,11 @@ class OC_Mount_Config { if (isset($mountConfig[self::MOUNT_TYPE_USER][$user])) { foreach ($mountConfig[self::MOUNT_TYPE_USER][$user] as $mountPoint => $options) { $options['options'] = self::decryptPasswords($options['options']); - \OC\Files\Filesystem::mount($options['class'], $options['options'], $mountPoint); + $mountPoints[$mountPoint] = $options; } } + + return $mountPoints; } /** diff --git a/apps/files_external/tests/mountconfig.php b/apps/files_external/tests/mountconfig.php index 8c255769fc..cfe04f66ce 100644 --- a/apps/files_external/tests/mountconfig.php +++ b/apps/files_external/tests/mountconfig.php @@ -345,4 +345,114 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { $savedMountConfig = $config['ext']['configuration']; $this->assertEquals($mountConfig, $savedMountConfig); } + + public function mountDataProvider() { + return array( + // Tests for visible mount points + // system mount point for all users + array( + false, + OC_Mount_Config::MOUNT_TYPE_USER, + 'all', + 'test', + true, + ), + // system mount point for a specific user + array( + false, + OC_Mount_Config::MOUNT_TYPE_USER, + 'test', + 'test', + true, + ), + // system mount point for a specific group + array( + false, + OC_Mount_Config::MOUNT_TYPE_GROUP, + 'testgroup1', + 'test', + true, + ), + // user mount point + array( + true, + OC_Mount_Config::MOUNT_TYPE_USER, + 'test', + 'test', + true, + ), + + // Tests for non-visible mount points + // system mount point for another user + array( + false, + OC_Mount_Config::MOUNT_TYPE_USER, + 'anotheruser', + 'test', + false, + ), + // system mount point for a specific group + array( + false, + OC_Mount_Config::MOUNT_TYPE_GROUP, + 'anothergroup', + 'test', + false, + ), + // user mount point + array( + true, + OC_Mount_Config::MOUNT_TYPE_USER, + 'test', + 'anotheruser', + false, + ), + ); + } + + /** + * Test mount points used at mount time, making sure + * the configuration is prepared properly. + * + * @dataProvider mountDataProvider + * @param bool $isPersonal true for personal mount point, false for system mount point + * @param string $mountType mount type + * @param string $applicable target user/group or "all" + * @param string $testUser user for which to retrieve the mount points + * @param bool $expectVisible whether to expect the mount point to be visible for $testUser + */ + public function testMount($isPersonal, $mountType, $applicable, $testUser, $expectVisible) { + $mountConfig = array( + 'host' => 'someost', + 'user' => 'someuser', + 'password' => 'somepassword', + 'root' => 'someroot' + ); + + // add mount point as "test" user + $this->assertTrue( + OC_Mount_Config::addMountPoint( + '/ext', + '\OC\Files\Storage\SMB', + $mountConfig, + $mountType, + $applicable, + $isPersonal + ) + ); + + // check mount points in the perspective of user $testUser + \OC_User::setUserId($testUser); + + $mountPoints = OC_Mount_Config::getAbsoluteMountPoints($testUser); + if ($expectVisible) { + $this->assertEquals(1, count($mountPoints)); + $this->assertTrue(isset($mountPoints['/test/files/ext'])); + $this->assertEquals('\OC\Files\Storage\SMB', $mountPoints['/test/files/ext']['class']); + $this->assertEquals($mountConfig, $mountPoints['/test/files/ext']['options']); + } + else { + $this->assertEquals(0, count($mountPoints)); + } + } } From 5b6c36347b63909f09a2738e96cbc0ae23fd4c61 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 19 Mar 2014 17:55:34 +0100 Subject: [PATCH 06/10] Fixed ext storage unit test for groups Now creating real users and groups for testing external storage mounts --- apps/files_external/tests/mountconfig.php | 76 ++++++++++++++--------- 1 file changed, 46 insertions(+), 30 deletions(-) diff --git a/apps/files_external/tests/mountconfig.php b/apps/files_external/tests/mountconfig.php index cfe04f66ce..144aad4f64 100644 --- a/apps/files_external/tests/mountconfig.php +++ b/apps/files_external/tests/mountconfig.php @@ -40,9 +40,22 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { private $oldAllowedBackends; private $allBackends; + const TEST_USER1 = 'user1'; + const TEST_USER2 = 'user2'; + const TEST_GROUP1 = 'group1'; + const TEST_GROUP2 = 'group2'; + public function setUp() { - \OC_User::setUserId('test'); - $this->userHome = \OC_User::getHome('test'); + \OC_User::createUser(self::TEST_USER1, self::TEST_USER1); + \OC_User::createUser(self::TEST_USER2, self::TEST_USER2); + + \OC_Group::createGroup(self::TEST_GROUP1); + \OC_Group::addToGroup(self::TEST_USER1, self::TEST_GROUP1); + \OC_Group::createGroup(self::TEST_GROUP2); + \OC_Group::addToGroup(self::TEST_USER2, self::TEST_GROUP2); + + \OC_User::setUserId(self::TEST_USER1); + $this->userHome = \OC_User::getHome(self::TEST_USER1); mkdir($this->userHome); $this->dataDir = \OC_Config::getValue( @@ -67,9 +80,12 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { public function tearDown() { OC_Mount_Config::$skipTest = false; + \OC_User::deleteUser(self::TEST_USER2); + \OC_User::deleteUser(self::TEST_USER1); + \OC_Group::deleteGroup(self::TEST_GROUP1); + \OC_Group::deleteGroup(self::TEST_GROUP2); + @unlink($this->dataDir . '/mount.json'); - @unlink($this->userHome . '/mount.json'); - rmdir($this->userHome); OCP\Config::setAppValue( 'files_external', @@ -143,7 +159,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { */ public function testAddMountPointSingleUser() { $mountType = OC_Mount_Config::MOUNT_TYPE_USER; - $applicable = 'test'; + $applicable = self::TEST_USER1; $isPersonal = true; $this->assertEquals(true, OC_Mount_Config::addMountPoint('/ext', '\OC\Files\Storage\SFTP', array(), $mountType, $applicable, $isPersonal)); @@ -152,10 +168,10 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { $this->assertEquals(1, count($config)); $this->assertTrue(isset($config[$mountType])); $this->assertTrue(isset($config[$mountType][$applicable])); - $this->assertTrue(isset($config[$mountType][$applicable]['/test/files/ext'])); + $this->assertTrue(isset($config[$mountType][$applicable]['/' . self::TEST_USER1 . '/files/ext'])); $this->assertEquals( '\OC\Files\Storage\SFTP', - $config[$mountType][$applicable]['/test/files/ext']['class'] + $config[$mountType][$applicable]['/' . self::TEST_USER1 . '/files/ext']['class'] ); } @@ -164,7 +180,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { */ public function testAddDisallowedBackendMountPointSingleUser() { $mountType = OC_Mount_Config::MOUNT_TYPE_USER; - $applicable = 'test'; + $applicable = self::TEST_USER1; $isPersonal = true; // local @@ -189,7 +205,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { public function testAddMountPointUnexistClass() { $storageClass = 'Unexist_Storage'; $mountType = OC_Mount_Config::MOUNT_TYPE_USER; - $applicable = 'test'; + $applicable = self::TEST_USER1; $isPersonal = false; $this->assertFalse(OC_Mount_Config::addMountPoint('/ext', $storageClass, array(), $mountType, $applicable, $isPersonal)); @@ -236,7 +252,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { */ public function testReadWritePersonalConfig() { $mountType = OC_Mount_Config::MOUNT_TYPE_USER; - $applicable = 'test'; + $applicable = self::TEST_USER1; $isPersonal = true; $mountConfig = array( 'host' => 'smbhost', @@ -272,7 +288,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { */ public function testPasswordObfuscation() { $mountType = OC_Mount_Config::MOUNT_TYPE_USER; - $applicable = 'test'; + $applicable = self::TEST_USER1; $isPersonal = true; $mountConfig = array( 'host' => 'smbhost', @@ -298,7 +314,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { // check that password inside the file is NOT in plain text $config = $this->readUserConfig(); - $savedConfig = $config[$mountType][$applicable]['/test/files/ext']['options']; + $savedConfig = $config[$mountType][$applicable]['/' . self::TEST_USER1 . '/files/ext']['options']; // no more clear text password in file $this->assertFalse(isset($savedConfig['password'])); @@ -312,7 +328,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { */ public function testReadLegacyPassword() { $mountType = OC_Mount_Config::MOUNT_TYPE_USER; - $applicable = 'test'; + $applicable = self::TEST_USER1; $isPersonal = true; $mountConfig = array( 'host' => 'smbhost', @@ -336,7 +352,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { $config = $this->readUserConfig(); // simulate non-encrypted password situation - $config[$mountType][$applicable]['/test/files/ext']['options']['password'] = 'smbpasswd'; + $config[$mountType][$applicable]['/' . self::TEST_USER1 . '/files/ext']['options']['password'] = 'smbpasswd'; $this->writeUserConfig($config); @@ -354,31 +370,31 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { false, OC_Mount_Config::MOUNT_TYPE_USER, 'all', - 'test', + self::TEST_USER1, true, ), // system mount point for a specific user array( false, OC_Mount_Config::MOUNT_TYPE_USER, - 'test', - 'test', + self::TEST_USER1, + self::TEST_USER1, true, ), // system mount point for a specific group array( false, OC_Mount_Config::MOUNT_TYPE_GROUP, - 'testgroup1', - 'test', + self::TEST_GROUP1, + self::TEST_USER1, true, ), // user mount point array( true, OC_Mount_Config::MOUNT_TYPE_USER, - 'test', - 'test', + self::TEST_USER1, + self::TEST_USER1, true, ), @@ -387,24 +403,24 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { array( false, OC_Mount_Config::MOUNT_TYPE_USER, - 'anotheruser', - 'test', + self::TEST_USER2, + self::TEST_USER1, false, ), // system mount point for a specific group array( false, OC_Mount_Config::MOUNT_TYPE_GROUP, - 'anothergroup', - 'test', + self::TEST_GROUP2, + self::TEST_USER1, false, ), // user mount point array( true, OC_Mount_Config::MOUNT_TYPE_USER, - 'test', - 'anotheruser', + self::TEST_USER1, + self::TEST_USER2, false, ), ); @@ -447,9 +463,9 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { $mountPoints = OC_Mount_Config::getAbsoluteMountPoints($testUser); if ($expectVisible) { $this->assertEquals(1, count($mountPoints)); - $this->assertTrue(isset($mountPoints['/test/files/ext'])); - $this->assertEquals('\OC\Files\Storage\SMB', $mountPoints['/test/files/ext']['class']); - $this->assertEquals($mountConfig, $mountPoints['/test/files/ext']['options']); + $this->assertTrue(isset($mountPoints['/' . self::TEST_USER1 . '/files/ext'])); + $this->assertEquals('\OC\Files\Storage\SMB', $mountPoints['/' . self::TEST_USER1 . '/files/ext']['class']); + $this->assertEquals($mountConfig, $mountPoints['/' . self::TEST_USER1 . '/files/ext']['options']); } else { $this->assertEquals(0, count($mountPoints)); From 9116c39a829ef10745acea5f3e9a4d47433ffa47 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 19 Mar 2014 17:56:36 +0100 Subject: [PATCH 07/10] Fixed ext storage password field order issue The old password field is now preserved in the JSON structure to make sure that the order is preserved. This is a quick fix until the UI is fixed to not rely on the PHP array key order. --- apps/files_external/lib/config.php | 5 ++++- apps/files_external/tests/mountconfig.php | 8 ++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/apps/files_external/lib/config.php b/apps/files_external/lib/config.php index 6de7c91358..9b2cb0d0b0 100755 --- a/apps/files_external/lib/config.php +++ b/apps/files_external/lib/config.php @@ -50,6 +50,7 @@ class OC_Mount_Config { */ public static function getBackends() { + // FIXME: do not rely on php key order for the options order in the UI $backends['\OC\Files\Storage\Local']=array( 'backend' => 'Local', 'configuration' => array( @@ -649,7 +650,9 @@ class OC_Mount_Config { private static function encryptPasswords($options) { if (isset($options['password'])) { $options['password_encrypted'] = self::encryptPassword($options['password']); - unset($options['password']); + // do not unset the password, we want to keep the keys order + // on load... because that's how the UI currently works + $options['password'] = ''; } return $options; } diff --git a/apps/files_external/tests/mountconfig.php b/apps/files_external/tests/mountconfig.php index 144aad4f64..bf43bb31c3 100644 --- a/apps/files_external/tests/mountconfig.php +++ b/apps/files_external/tests/mountconfig.php @@ -245,6 +245,8 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { $this->assertEquals('\OC\Files\Storage\SMB', $config['ext']['class']); $savedMountConfig = $config['ext']['configuration']; $this->assertEquals($mountConfig, $savedMountConfig); + // key order needs to be preserved for the UI... + $this->assertEquals(array_keys($mountConfig), array_keys($savedMountConfig)); } /** @@ -281,6 +283,8 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { $this->assertEquals('\OC\Files\Storage\SMB', $config['ext']['class']); $savedMountConfig = $config['ext']['configuration']; $this->assertEquals($mountConfig, $savedMountConfig); + // key order needs to be preserved for the UI... + $this->assertEquals(array_keys($mountConfig), array_keys($savedMountConfig)); } /** @@ -316,8 +320,8 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { $config = $this->readUserConfig(); $savedConfig = $config[$mountType][$applicable]['/' . self::TEST_USER1 . '/files/ext']['options']; - // no more clear text password in file - $this->assertFalse(isset($savedConfig['password'])); + // no more clear text password in file (kept because of key order) + $this->assertEquals('', $savedConfig['password']); // encrypted password is present $this->assertNotEquals($mountConfig['password'], $savedConfig['password_encrypted']); From cf23defa5274fdd7d73d7ca3b68c7e366f8f0706 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 20 Mar 2014 12:52:09 +0100 Subject: [PATCH 08/10] Fix typo in mount loading --- apps/files_external/lib/config.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/files_external/lib/config.php b/apps/files_external/lib/config.php index 9b2cb0d0b0..dbb12ecd9b 100755 --- a/apps/files_external/lib/config.php +++ b/apps/files_external/lib/config.php @@ -173,7 +173,7 @@ class OC_Mount_Config { */ public static function initMountPointsHook($data) { $mountPoints = self::getAbsoluteMountPoints($data['user']); - foreach ($mountPoints as $mountPoints => $options) { + foreach ($mountPoints as $mountPoint => $options) { \OC\Files\Filesystem::mount($options['class'], $options['options'], $mountPoint); } } From 1e4e0cfcd5ddbcb09a17a03d7525d8f3b8ce002f Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 20 Mar 2014 13:21:34 +0100 Subject: [PATCH 09/10] Lazy load phpseclib in ext storage config --- apps/files_external/lib/config.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/files_external/lib/config.php b/apps/files_external/lib/config.php index dbb12ecd9b..f13b25b2e2 100755 --- a/apps/files_external/lib/config.php +++ b/apps/files_external/lib/config.php @@ -24,7 +24,6 @@ set_include_path( get_include_path() . PATH_SEPARATOR . \OC_App::getAppPath('files_external') . '/3rdparty/phpseclib/phpseclib' ); -include('Crypt/AES.php'); /** * Class to configure mount.json globally and for users @@ -703,6 +702,9 @@ class OC_Mount_Config { private static function getCipher() { // note: not caching this to make it thread safe as we'll use // a different IV for each password + if (!class_exists('Crypt_AES', false)) { + include('Crypt/AES.php'); + } $cipher = new Crypt_AES(CRYPT_AES_MODE_CBC); $cipher->setKey(\OCP\Config::getSystemValue('passwordsalt')); return $cipher; From d95fde39248e8c4083c14788fe3a327a60611b47 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 20 Mar 2014 15:24:05 +0100 Subject: [PATCH 10/10] Removed comment --- apps/files_external/lib/config.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/apps/files_external/lib/config.php b/apps/files_external/lib/config.php index f13b25b2e2..ffbab7bca8 100755 --- a/apps/files_external/lib/config.php +++ b/apps/files_external/lib/config.php @@ -700,8 +700,6 @@ class OC_Mount_Config { * Returns the encryption cipher */ private static function getCipher() { - // note: not caching this to make it thread safe as we'll use - // a different IV for each password if (!class_exists('Crypt_AES', false)) { include('Crypt/AES.php'); }