Merge pull request #7888 from owncloud/extstorage-multiplemountpointconfig

Fix merging of external storage configurations

3 files changed, 242 insertions, 51 deletions

diff --git a/apps/files_external/lib/config.php b/apps/files_external/lib/config.php
index c5b091336e1..f7caafb74aa 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'])
 				);
 			}
@@ -712,4 +722,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 @@
 			</thead>
 			<tbody width="100%">
 			<?php $_['mounts'] = array_merge($_['mounts'], array('' => array())); ?>
-			<?php foreach ($_['mounts'] as $mountPoint => $mount): ?>
-				<tr <?php print_unescaped(($mountPoint != '') ? 'class="'.OC_Util::sanitizeHTML($mount['class']).'"' : 'id="addMountPoint"'); ?>>
+			<?php foreach ($_['mounts'] as $mount): ?>
+				<tr <?php print_unescaped(($mount['mountpoint'] !== '') ? 'class="'.OC_Util::sanitizeHTML($mount['class']).'"' : 'id="addMountPoint"'); ?>>
 					<td class="status">
 					<?php if (isset($mount['status'])): ?>
 						<span class="<?php p(($mount['status']) ? 'success' : 'error'); ?>"></span>
 					<?php endif; ?>
 					</td>
 					<td class="mountPoint"><input type="text" name="mountPoint"
-												  value="<?php p($mountPoint); ?>"
+												  value="<?php p($mount['mountpoint']); ?>"
 												  placeholder="<?php p($l->t('Folder name')); ?>" /></td>
-					<?php if ($mountPoint == ''): ?>
+					<?php if ($mount['mountpoint'] == ''): ?>
 						<td class="backend">
 							<select id="selectBackend" data-configurations='<?php print_unescaped(json_encode($_['backends'])); ?>'>
 								<option value="" disabled selected
@@ -41,8 +41,8 @@
 							data-class="<?php p($mount['class']); ?>"><?php p($mount['backend']); ?></td>
 					<?php endif; ?>
 					<td class ="configuration" width="100%">
-						<?php if (isset($mount['configuration'])): ?>
-							<?php foreach ($mount['configuration'] as $parameter => $value): ?>
+						<?php if (isset($mount['options'])): ?>
+							<?php foreach ($mount['options'] as $parameter => $value): ?>
 								<?php if (isset($_['backends'][$mount['class']]['configuration'][$parameter])): ?>
 									<?php $placeholder = $_['backends'][$mount['class']]['configuration'][$parameter]; ?>
 									<?php if (strpos($placeholder, '*') !== false): ?>
@@ -108,7 +108,7 @@
 							</select>
 						</td>
 					<?php endif; ?>
-					<td <?php if ($mountPoint != ''): ?>class="remove"
+					<td <?php if ($mount['mountpoint'] != ''): ?>class="remove"
 						<?php else: ?>style="visibility:hidden;"
 						<?php endif ?>><img alt="<?php p($l->t('Delete')); ?>"
 											title="<?php p($l->t('Delete')); ?>"
diff --git a/apps/files_external/tests/mountconfig.php b/apps/files_external/tests/mountconfig.php
index bf43bb31c38..c89874c94d5 100644
--- a/apps/files_external/tests/mountconfig.php
+++ b/apps/files_external/tests/mountconfig.php
@@ -212,13 +212,51 @@ class Test_Mount_Config extends \PHPUnit_Framework_TestCase {
 	}
 
 	/**
+	 * Provider for testing configurations with different
+	 * "applicable" values (all, user, groups)
+	 */
+	public function applicableConfigProvider() {
+		return array(
+			// applicable to "all"
+			array(
+				OC_Mount_Config::MOUNT_TYPE_USER,
+				'all',
+				array(
+					'users' => 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']);
+	}
 }