From e002b7242cb19a0e028d325cd64b57e67dc48108 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Wed, 26 Mar 2014 12:10:17 +0100 Subject: [PATCH] Fix merging of external storage configurations Merging of configurations is whenever the same config is available for multiple users/groups, in which case the config is considered as a single one by the UI, and shows multiple users/groups selected. Fixed merging logic to make sure that class, mount point and options are the same before merging them. Fixed merging to work correctly when the same mount point path is used for separate users and configs. These are now correctly shows in the UI as separate entries. --- apps/files_external/lib/config.php | 78 +++++--- apps/files_external/templates/settings.php | 14 +- apps/files_external/tests/mountconfig.php | 201 +++++++++++++++++++-- 3 files changed, 242 insertions(+), 51 deletions(-) diff --git a/apps/files_external/lib/config.php b/apps/files_external/lib/config.php index ffbab7bca80..384ab410e9c 100755 --- a/apps/files_external/lib/config.php +++ b/apps/files_external/lib/config.php @@ -304,18 +304,23 @@ class OC_Mount_Config { $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 - if (isset($system[$mountPoint]) && $system[$mountPoint]['configuration'] == $mount['options']) { - $system[$mountPoint]['applicable']['groups'] - = array_merge($system[$mountPoint]['applicable']['groups'], array($group)); + + $config = array( + 'class' => $mount['class'], + 'mountpoint' => $mountPoint, + 'backend' => $backends[$mount['class']]['backend'], + 'options' => $mount['options'], + 'applicable' => array('groups' => array($group), 'users' => array()), + 'status' => self::getBackendStatus($mount['class'], $mount['options']) + ); + $hash = self::makeConfigHash($config); + // If an existing config exists (with same class, mountpoint and options) + if (isset($system[$hash])) { + // add the groups into that config + $system[$hash]['applicable']['groups'] + = array_merge($system[$hash]['applicable']['groups'], array($group)); } else { - $system[$mountPoint] = array( - 'class' => $mount['class'], - 'backend' => $backends[$mount['class']]['backend'], - 'configuration' => $mount['options'], - 'applicable' => array('groups' => array($group), 'users' => array()), - 'status' => self::getBackendStatus($mount['class'], $mount['options']) - ); + $system[$hash] = $config; } } } @@ -330,23 +335,27 @@ class OC_Mount_Config { $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 - if (isset($system[$mountPoint]) && $system[$mountPoint]['configuration'] == $mount['options']) { - $system[$mountPoint]['applicable']['users'] - = array_merge($system[$mountPoint]['applicable']['users'], array($user)); + $config = array( + 'class' => $mount['class'], + 'mountpoint' => $mountPoint, + 'backend' => $backends[$mount['class']]['backend'], + 'options' => $mount['options'], + 'applicable' => array('groups' => array(), 'users' => array($user)), + 'status' => self::getBackendStatus($mount['class'], $mount['options']) + ); + $hash = self::makeConfigHash($config); + // If an existing config exists (with same class, mountpoint and options) + if (isset($system[$hash])) { + // add the users into that config + $system[$hash]['applicable']['users'] + = array_merge($system[$hash]['applicable']['users'], array($user)); } else { - $system[$mountPoint] = array( - 'class' => $mount['class'], - 'backend' => $backends[$mount['class']]['backend'], - 'configuration' => $mount['options'], - 'applicable' => array('groups' => array(), 'users' => array($user)), - 'status' => self::getBackendStatus($mount['class'], $mount['options']) - ); + $system[$hash] = $config; } } } } - return $system; + return array_values($system); } /** @@ -366,11 +375,12 @@ class OC_Mount_Config { $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( + $personal[] = array( 'class' => $mount['class'], + // Remove '/uid/files/' from mount point + 'mountpoint' => substr($mountPoint, strlen($uid) + 8), 'backend' => $backends[$mount['class']]['backend'], - 'configuration' => $mount['options'], + 'options' => $mount['options'], 'status' => self::getBackendStatus($mount['class'], $mount['options']) ); } @@ -707,4 +717,20 @@ class OC_Mount_Config { $cipher->setKey(\OCP\Config::getSystemValue('passwordsalt')); return $cipher; } + + /** + * Computes a hash based on the given configuration. + * This is mostly used to find out whether configurations + * are the same. + */ + private static function makeConfigHash($config) { + $data = json_encode( + array( + 'c' => $config['class'], + 'm' => $config['mountpoint'], + 'o' => $config['options'] + ) + ); + return hash('md5', $data); + } } diff --git a/apps/files_external/templates/settings.php b/apps/files_external/templates/settings.php index de44d3c8644..5e84fa8a252 100644 --- a/apps/files_external/templates/settings.php +++ b/apps/files_external/templates/settings.php @@ -16,17 +16,17 @@ array())); ?> - $mount): ?> - > + + > - + - class="remove" + class="remove" style="visibility:hidden;" ><?php p($l->t('Delete')); ?> array('all'), + 'groups' => array() + ) + ), + // applicable to single user + array( + OC_Mount_Config::MOUNT_TYPE_USER, + self::TEST_USER1, + array( + 'users' => array(self::TEST_USER1), + 'groups' => array() + ) + ), + // applicable to single group + array( + OC_Mount_Config::MOUNT_TYPE_GROUP, + self::TEST_GROUP1, + array( + 'users' => array(), + 'groups' => array(self::TEST_GROUP1) + ) + ), + ); + } + /** * Test reading and writing global config + * + * @dataProvider applicableConfigProvider */ - public function testReadWriteGlobalConfig() { - $mountType = OC_Mount_Config::MOUNT_TYPE_USER; - $applicable = 'all'; + public function testReadWriteGlobalConfig($mountType, $applicable, $expectApplicableArray) { + $mountType = $mountType; + $applicable = $applicable; $isPersonal = false; - $mountConfig = array( + $options = array( 'host' => 'smbhost', 'user' => 'smbuser', 'password' => 'smbpassword', @@ -231,7 +269,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { OC_Mount_Config::addMountPoint( '/ext', '\OC\Files\Storage\SMB', - $mountConfig, + $options, $mountType, $applicable, $isPersonal @@ -241,12 +279,13 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { // 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); + $this->assertEquals('\OC\Files\Storage\SMB', $config[0]['class']); + $this->assertEquals('ext', $config[0]['mountpoint']); + $this->assertEquals($expectApplicableArray, $config[0]['applicable']); + $savedOptions = $config[0]['options']; + $this->assertEquals($options, $savedOptions); // key order needs to be preserved for the UI... - $this->assertEquals(array_keys($mountConfig), array_keys($savedMountConfig)); + $this->assertEquals(array_keys($options), array_keys($savedOptions)); } /** @@ -256,7 +295,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { $mountType = OC_Mount_Config::MOUNT_TYPE_USER; $applicable = self::TEST_USER1; $isPersonal = true; - $mountConfig = array( + $options = array( 'host' => 'smbhost', 'user' => 'smbuser', 'password' => 'smbpassword', @@ -269,7 +308,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { OC_Mount_Config::addMountPoint( '/ext', '\OC\Files\Storage\SMB', - $mountConfig, + $options, $mountType, $applicable, $isPersonal @@ -279,12 +318,12 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { // 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); + $this->assertEquals('\OC\Files\Storage\SMB', $config[0]['class']); + $this->assertEquals('ext', $config[0]['mountpoint']); + $savedOptions = $config[0]['options']; + $this->assertEquals($options, $savedOptions); // key order needs to be preserved for the UI... - $this->assertEquals(array_keys($mountConfig), array_keys($savedMountConfig)); + $this->assertEquals(array_keys($options), array_keys($savedOptions)); } /** @@ -362,7 +401,7 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { // re-read config, password was read correctly $config = OC_Mount_Config::getPersonalMountPoints(); - $savedMountConfig = $config['ext']['configuration']; + $savedMountConfig = $config[0]['options']; $this->assertEquals($mountConfig, $savedMountConfig); } @@ -475,4 +514,130 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase { $this->assertEquals(0, count($mountPoints)); } } + + /** + * Test the same config for multiple users. + * The config will be merged by getSystemMountPoints(). + */ + public function testConfigMerging() { + $mountType = OC_Mount_Config::MOUNT_TYPE_USER; + $isPersonal = false; + $options = array( + 'host' => 'smbhost', + 'user' => 'smbuser', + 'password' => 'smbpassword', + 'share' => 'smbshare', + 'root' => 'smbroot' + ); + + // write config + $this->assertTrue( + OC_Mount_Config::addMountPoint( + '/ext', + '\OC\Files\Storage\SMB', + $options, + OC_Mount_Config::MOUNT_TYPE_USER, + self::TEST_USER1, + $isPersonal + ) + ); + + $this->assertTrue( + OC_Mount_Config::addMountPoint( + '/ext', + '\OC\Files\Storage\SMB', + $options, + OC_Mount_Config::MOUNT_TYPE_USER, + self::TEST_USER2, + $isPersonal + ) + ); + + $this->assertTrue( + OC_Mount_Config::addMountPoint( + '/ext', + '\OC\Files\Storage\SMB', + $options, + OC_Mount_Config::MOUNT_TYPE_GROUP, + self::TEST_GROUP2, + $isPersonal + ) + ); + + $this->assertTrue( + OC_Mount_Config::addMountPoint( + '/ext', + '\OC\Files\Storage\SMB', + $options, + OC_Mount_Config::MOUNT_TYPE_GROUP, + self::TEST_GROUP1, + $isPersonal + ) + ); + + // re-read config + $config = OC_Mount_Config::getSystemMountPoints(); + $this->assertEquals(1, count($config)); + $this->assertEquals('\OC\Files\Storage\SMB', $config[0]['class']); + $this->assertEquals('ext', $config[0]['mountpoint']); + $this->assertEquals($options, $config[0]['options']); + $this->assertEquals(array(self::TEST_USER1, self::TEST_USER2), $config[0]['applicable']['users']); + $this->assertEquals(array(self::TEST_GROUP2, self::TEST_GROUP1), $config[0]['applicable']['groups']); + } + + /** + * Create then re-read mount points configs where the mount points + * have the same path, the config must NOT be merged. + */ + public function testRereadMountpointWithSamePath() { + $mountType = OC_Mount_Config::MOUNT_TYPE_USER; + $isPersonal = false; + $options1 = array( + 'host' => 'smbhost', + 'user' => 'smbuser', + 'password' => 'smbpassword', + 'share' => 'smbshare', + 'root' => 'smbroot' + ); + + // write config + $this->assertTrue( + OC_Mount_Config::addMountPoint( + '/ext', + '\OC\Files\Storage\SMB', + $options1, + $mountType, + self::TEST_USER1, + $isPersonal + ) + ); + + $options2 = array( + 'host' => 'anothersmbhost', + 'user' => 'anothersmbuser', + 'password' => 'anothersmbpassword', + 'share' => 'anothersmbshare', + 'root' => 'anothersmbroot' + ); + $this->assertTrue( + OC_Mount_Config::addMountPoint( + '/ext', + '\OC\Files\Storage\SMB', + $options2, + $mountType, + self::TEST_USER2, + $isPersonal + ) + ); + + // re-read config + $config = OC_Mount_Config::getSystemMountPoints(); + $this->assertEquals(2, count($config)); + $this->assertEquals('\OC\Files\Storage\SMB', $config[0]['class']); + $this->assertEquals('ext', $config[0]['mountpoint']); + $this->assertEquals($options1, $config[0]['options']); + $this->assertEquals('\OC\Files\Storage\SMB', $config[1]['class']); + $this->assertEquals('ext', $config[1]['mountpoint']); + $this->assertEquals($options2, $config[1]['options']); + } } -- 2.39.5