Merge pull request #18445 from owncloud/ext-only-setuservars-string

setUserVars() should only attempt substitution with strings

11 files changed, 83 insertions, 22 deletions

diff --git a/apps/files_external/lib/amazons3.php b/apps/files_external/lib/amazons3.php
index fd98d26f834..7c6ac4cbdd5 100644
--- a/apps/files_external/lib/amazons3.php
+++ b/apps/files_external/lib/amazons3.php
@@ -122,7 +122,7 @@ class AmazonS3 extends \OC\Files\Storage\Common {
 		$params['region'] = empty($params['region']) ? 'eu-west-1' : $params['region'];
 		$params['hostname'] = empty($params['hostname']) ? 's3.amazonaws.com' : $params['hostname'];
 		if (!isset($params['port']) || $params['port'] === '') {
-			$params['port'] = ($params['use_ssl'] === 'false') ? 80 : 443;
+			$params['port'] = ($params['use_ssl'] === false) ? 80 : 443;
 		}
 		$this->params = $params;
 	}
@@ -585,7 +585,7 @@ class AmazonS3 extends \OC\Files\Storage\Common {
 			return $this->connection;
 		}
 
-		$scheme = ($this->params['use_ssl'] === 'false') ? 'http' : 'https';
+		$scheme = ($this->params['use_ssl'] === false) ? 'http' : 'https';
 		$base_url = $scheme . '://' . $this->params['hostname'] . ':' . $this->params['port'] . '/';
 
 		$this->connection = S3Client::factory(array(
diff --git a/apps/files_external/lib/config.php b/apps/files_external/lib/config.php
index b4c6867a26c..62932dc4028 100644
--- a/apps/files_external/lib/config.php
+++ b/apps/files_external/lib/config.php
@@ -230,7 +230,9 @@ class OC_Mount_Config {
 				}
 			}
 		} else {
-			$input = str_replace('$user', $user, $input);
+			if (is_string($input)) {
+				$input = str_replace('$user', $user, $input);
+			}
 		}
 		return $input;
 	}
diff --git a/apps/files_external/lib/definitionparameter.php b/apps/files_external/lib/definitionparameter.php
index 4b560908b69..7f883e5fad1 100644
--- a/apps/files_external/lib/definitionparameter.php
+++ b/apps/files_external/lib/definitionparameter.php
@@ -154,22 +154,31 @@ class DefinitionParameter implements \JsonSerializable {
 
 	/**
 	 * Validate a parameter value against this
+	 * Convert type as necessary
 	 *
 	 * @param mixed $value Value to check
 	 * @return bool success
 	 */
-	public function validateValue($value) {
-		if ($this->getFlags() & self::FLAG_OPTIONAL) {
-			return true;
-		}
+	public function validateValue(&$value) {
+		$optional = $this->getFlags() & self::FLAG_OPTIONAL;
+
 		switch ($this->getType()) {
 		case self::VALUE_BOOLEAN:
 			if (!is_bool($value)) {
-				return false;
+				switch ($value) {
+				case 'true':
+					$value = true;
+					break;
+				case 'false':
+					$value = false;
+					break;
+				default:
+					return false;
+				}
 			}
 			break;
 		default:
-			if (empty($value)) {
+			if (!$value && !$optional) {
 				return false;
 			}
 			break;
diff --git a/apps/files_external/lib/frontenddefinitiontrait.php b/apps/files_external/lib/frontenddefinitiontrait.php
index 4b826372d2f..a5fb1a3f62f 100644
--- a/apps/files_external/lib/frontenddefinitiontrait.php
+++ b/apps/files_external/lib/frontenddefinitiontrait.php
@@ -134,12 +134,12 @@ trait FrontendDefinitionTrait {
 	 * @return bool
 	 */
 	public function validateStorageDefinition(StorageConfig $storage) {
-		$options = $storage->getBackendOptions();
 		foreach ($this->getParameters() as $name => $parameter) {
-			$value = isset($options[$name]) ? $options[$name] : null;
+			$value = $storage->getBackendOption($name);
 			if (!$parameter->validateValue($value)) {
 				return false;
 			}
+			$storage->setBackendOption($name, $value);
 		}
 		return true;
 	}
diff --git a/apps/files_external/lib/ftp.php b/apps/files_external/lib/ftp.php
index aeca17c4f4c..f3631e53fa1 100644
--- a/apps/files_external/lib/ftp.php
+++ b/apps/files_external/lib/ftp.php
@@ -45,11 +45,7 @@ class FTP extends \OC\Files\Storage\StreamWrapper{
 			$this->user=$params['user'];
 			$this->password=$params['password'];
 			if (isset($params['secure'])) {
-				if (is_string($params['secure'])) {
-					$this->secure = ($params['secure'] === 'true');
-				} else {
-					$this->secure = (bool)$params['secure'];
-				}
+				$this->secure = $params['secure'];
 			} else {
 				$this->secure = false;
 			}
diff --git a/apps/files_external/lib/smb_oc.php b/apps/files_external/lib/smb_oc.php
index 52b73c46ff9..547caa5ecbf 100644
--- a/apps/files_external/lib/smb_oc.php
+++ b/apps/files_external/lib/smb_oc.php
@@ -39,7 +39,7 @@ class SMB_OC extends SMB {
 	public function __construct($params) {
 		if (isset($params['host'])) {
 			$host = $params['host'];
-			$this->username_as_share = ($params['username_as_share'] === 'true');
+			$this->username_as_share = ($params['username_as_share'] === true);
 
 			// dummy credentials, unused, to satisfy constructor
 			$user = 'foo';
diff --git a/apps/files_external/service/storagesservice.php b/apps/files_external/service/storagesservice.php
index 282ac8f2db1..3e2152741e5 100644
--- a/apps/files_external/service/storagesservice.php
+++ b/apps/files_external/service/storagesservice.php
@@ -118,6 +118,8 @@ abstract class StoragesService {
 			$applicableGroups[] = $applicable;
 			$storageConfig->setApplicableGroups($applicableGroups);
 		}
+
+
 		return $storageConfig;
 	}
 
@@ -238,6 +240,12 @@ abstract class StoragesService {
 			$this->setRealStorageIds($storages, $storagesWithConfigHash);
 		}
 
+		// convert parameter values
+		foreach ($storages as $storage) {
+			$storage->getBackend()->validateStorageDefinition($storage);
+			$storage->getAuthMechanism()->validateStorageDefinition($storage);
+		}
+
 		return $storages;
 	}
 
diff --git a/apps/files_external/templates/settings.php b/apps/files_external/templates/settings.php
index 611c5807a5f..5b79edf6cf7 100644
--- a/apps/files_external/templates/settings.php
+++ b/apps/files_external/templates/settings.php
@@ -27,7 +27,7 @@
 				<input type="checkbox"
 					<?php if (!empty($classes)): ?> class="<?php p(implode(' ', $classes)); ?>"<?php endif; ?>
 					data-parameter="<?php p($parameter->getName()); ?>"
-				 	<?php if ($value == 'true'): ?> checked="checked"<?php endif; ?>
+				 	<?php if ($value === true): ?> checked="checked"<?php endif; ?>
 				/>
 				<?php p($placeholder); ?>
 			</label>
diff --git a/apps/files_external/tests/definitionparameterttest.php b/apps/files_external/tests/definitionparameterttest.php
index 6be00508698..22e0b842254 100644
--- a/apps/files_external/tests/definitionparameterttest.php
+++ b/apps/files_external/tests/definitionparameterttest.php
@@ -49,6 +49,9 @@ class DefinitionParameterTest extends \Test\TestCase {
 
 			[Param::VALUE_BOOLEAN, Param::FLAG_NONE, false, true],
 			[Param::VALUE_BOOLEAN, Param::FLAG_NONE, 123, false],
+			// conversion from string to boolean
+			[Param::VALUE_BOOLEAN, Param::FLAG_NONE, 'false', true, false],
+			[Param::VALUE_BOOLEAN, Param::FLAG_NONE, 'true', true, true],
 
 			[Param::VALUE_PASSWORD, Param::FLAG_NONE, 'foobar', true],
 			[Param::VALUE_PASSWORD, Param::FLAG_NONE, '', false],
@@ -60,11 +63,14 @@ class DefinitionParameterTest extends \Test\TestCase {
 	/**
 	 * @dataProvider validateValueProvider
 	 */
-	public function testValidateValue($type, $flags, $value, $success) {
+	public function testValidateValue($type, $flags, $value, $success, $expectedValue = null) {
 		$param = new Param('foo', 'bar');
 		$param->setType($type);
 		$param->setFlags($flags);
 
 		$this->assertEquals($success, $param->validateValue($value));
+		if (isset($expectedValue)) {
+			$this->assertEquals($expectedValue, $value);
+		}
 	}
 }
diff --git a/apps/files_external/tests/frontenddefinitiontraittest.php b/apps/files_external/tests/frontenddefinitiontraittest.php
index 871a87d4c52..d92d02b9854 100644
--- a/apps/files_external/tests/frontenddefinitiontraittest.php
+++ b/apps/files_external/tests/frontenddefinitiontraittest.php
@@ -70,9 +70,11 @@ class FrontendDefinitionTraitTest extends \Test\TestCase {
 		$storageConfig = $this->getMockBuilder('\OCA\Files_External\Lib\StorageConfig')
 			->disableOriginalConstructor()
 			->getMock();
-		$storageConfig->expects($this->once())
-			->method('getBackendOptions')
-			->willReturn([]);
+		$storageConfig->expects($this->any())
+			->method('getBackendOption')
+			->willReturn(null);
+		$storageConfig->expects($this->any())
+			->method('setBackendOption');
 
 		$trait = $this->getMockForTrait('\OCA\Files_External\Lib\FrontendDefinitionTrait');
 		$trait->setText('test');
@@ -80,4 +82,35 @@ class FrontendDefinitionTraitTest extends \Test\TestCase {
 
 		$this->assertEquals($expectedSuccess, $trait->validateStorageDefinition($storageConfig));
 	}
+
+	public function testValidateStorageSet() {
+		$param = $this->getMockBuilder('\OCA\Files_External\Lib\DefinitionParameter')
+			->disableOriginalConstructor()
+			->getMock();
+		$param->method('getName')
+			->willReturn('param');
+		$param->expects($this->once())
+			->method('validateValue')
+			->will($this->returnCallback(function(&$value) {
+				$value = 'foobar';
+				return true;
+			}));
+
+		$storageConfig = $this->getMockBuilder('\OCA\Files_External\Lib\StorageConfig')
+			->disableOriginalConstructor()
+			->getMock();
+		$storageConfig->expects($this->once())
+			->method('getBackendOption')
+			->with('param')
+			->willReturn('barfoo');
+		$storageConfig->expects($this->once())
+			->method('setBackendOption')
+			->with('param', 'foobar');
+
+		$trait = $this->getMockForTrait('\OCA\Files_External\Lib\FrontendDefinitionTrait');
+		$trait->setText('test');
+		$trait->addParameter($param);
+
+		$this->assertEquals(true, $trait->validateStorageDefinition($storageConfig));
+	}
 }
diff --git a/apps/files_external/tests/service/globalstoragesservicetest.php b/apps/files_external/tests/service/globalstoragesservicetest.php
index 05585d2c065..2bc480ca312 100644
--- a/apps/files_external/tests/service/globalstoragesservicetest.php
+++ b/apps/files_external/tests/service/globalstoragesservicetest.php
@@ -789,6 +789,13 @@ class GlobalStoragesServiceTest extends StoragesServiceTest {
 
 		file_put_contents($configFile, json_encode($json));
 
+		$this->backendService->getBackend('identifier:\OCA\Files_External\Lib\Backend\SMB')
+			->expects($this->exactly(4))
+			->method('validateStorageDefinition');
+		$this->backendService->getAuthMechanism('identifier:\Auth\Mechanism')
+			->expects($this->exactly(4))
+			->method('validateStorageDefinition');
+
 		$allStorages = $this->service->getAllStorages();
 
 		$this->assertCount(4, $allStorages);