Merge pull request #19313 from owncloud/ext-validate-mount

Validate storage backend and auth mechanism before mounting

15 files changed, 143 insertions, 40 deletions

diff --git a/apps/files_external/controller/globalstoragescontroller.php b/apps/files_external/controller/globalstoragescontroller.php
index 3686a6189b4..3c1f2022505 100644
--- a/apps/files_external/controller/globalstoragescontroller.php
+++ b/apps/files_external/controller/globalstoragescontroller.php
@@ -179,14 +179,5 @@ class GlobalStoragesController extends StoragesController {
 
 	}
 
-	/**
-	 * Get the visibility type for this controller, used in validation
-	 *
-	 * @return string BackendService::VISIBILITY_* constants
-	 */
-	protected function getVisibilityType() {
-		return BackendService::VISIBILITY_ADMIN;
-	}
-
 
 }
diff --git a/apps/files_external/controller/storagescontroller.php b/apps/files_external/controller/storagescontroller.php
index 71055fd1b9c..6a01112f8c5 100644
--- a/apps/files_external/controller/storagescontroller.php
+++ b/apps/files_external/controller/storagescontroller.php
@@ -153,7 +153,7 @@ abstract class StoragesController extends Controller {
 		$backend = $storage->getBackend();
 		/** @var AuthMechanism */
 		$authMechanism = $storage->getAuthMechanism();
-		if (!$backend || $backend->checkDependencies()) {
+		if ($backend->checkDependencies()) {
 			// invalid backend
 			return new DataResponse(
 				array(
@@ -165,7 +165,7 @@ abstract class StoragesController extends Controller {
 			);
 		}
 
-		if (!$backend->isVisibleFor($this->getVisibilityType())) {
+		if (!$backend->isVisibleFor($this->service->getVisibilityType())) {
 			// not permitted to use backend
 			return new DataResponse(
 				array(
@@ -176,7 +176,7 @@ abstract class StoragesController extends Controller {
 				Http::STATUS_UNPROCESSABLE_ENTITY
 			);
 		}
-		if (!$authMechanism->isVisibleFor($this->getVisibilityType())) {
+		if (!$authMechanism->isVisibleFor($this->service->getVisibilityType())) {
 			// not permitted to use auth mechanism
 			return new DataResponse(
 				array(
@@ -211,13 +211,6 @@ abstract class StoragesController extends Controller {
 	}
 
 	/**
-	 * Get the visibility type for this controller, used in validation
-	 *
-	 * @return string BackendService::VISIBILITY_* constants
-	 */
-	abstract protected function getVisibilityType();
-
-	/**
 	 * Check whether the given storage is available / valid.
 	 *
 	 * Note that this operation can be time consuming depending
diff --git a/apps/files_external/controller/userstoragescontroller.php b/apps/files_external/controller/userstoragescontroller.php
index fcbe692d79e..f39f8a85d2d 100644
--- a/apps/files_external/controller/userstoragescontroller.php
+++ b/apps/files_external/controller/userstoragescontroller.php
@@ -187,13 +187,4 @@ class UserStoragesController extends StoragesController {
 		return parent::destroy($id);
 	}
 
-	/**
-	 * Get the visibility type for this controller, used in validation
-	 *
-	 * @return string BackendService::VISIBILITY_* constants
-	 */
-	protected function getVisibilityType() {
-		return BackendService::VISIBILITY_PERSONAL;
-	}
-
 }
diff --git a/apps/files_external/lib/config.php b/apps/files_external/lib/config.php
index 2dc7de30a6c..afea7ead4e6 100644
--- a/apps/files_external/lib/config.php
+++ b/apps/files_external/lib/config.php
@@ -112,7 +112,7 @@ class OC_Mount_Config {
 	 * @param string $uid user
 	 * @return array of mount point string as key, mountpoint config as value
 	 *
-	 * @deprecated 8.2.0 use UserGlobalStoragesService::getAllStorages() and UserStoragesService::getAllStorages()
+	 * @deprecated 8.2.0 use UserGlobalStoragesService::getStorages() and UserStoragesService::getStorages()
 	 */
 	public static function getAbsoluteMountPoints($uid) {
 		$mountPoints = array();
@@ -124,7 +124,7 @@ class OC_Mount_Config {
 		$userGlobalStoragesService->setUser($user);
 		$userStoragesService->setUser($user);
 
-		foreach ($userGlobalStoragesService->getAllStorages() as $storage) {
+		foreach ($userGlobalStoragesService->getStorages() as $storage) {
 			$mountPoint = '/'.$uid.'/files'.$storage->getMountPoint();
 			$mountEntry = self::prepareMountPointEntry($storage, false);
 			foreach ($mountEntry['options'] as &$option) {
@@ -133,7 +133,7 @@ class OC_Mount_Config {
 			$mountPoints[$mountPoint] = $mountEntry;
 		}
 
-		foreach ($userStoragesService->getAllStorages() as $storage) {
+		foreach ($userStoragesService->getStorages() as $storage) {
 			$mountPoint = '/'.$uid.'/files'.$storage->getMountPoint();
 			$mountEntry = self::prepareMountPointEntry($storage, true);
 			foreach ($mountEntry['options'] as &$option) {
@@ -153,13 +153,13 @@ class OC_Mount_Config {
 	 *
 	 * @return array
 	 *
-	 * @deprecated 8.2.0 use GlobalStoragesService::getAllStorages()
+	 * @deprecated 8.2.0 use GlobalStoragesService::getStorages()
 	 */
 	public static function getSystemMountPoints() {
 		$mountPoints = [];
 		$service = self::$app->getContainer()->query('OCA\Files_External\Service\GlobalStoragesService');
 
-		foreach ($service->getAllStorages() as $storage) {
+		foreach ($service->getStorages() as $storage) {
 			$mountPoints[] = self::prepareMountPointEntry($storage, false);
 		}
 
@@ -171,13 +171,13 @@ class OC_Mount_Config {
 	 *
 	 * @return array
 	 *
-	 * @deprecated 8.2.0 use UserStoragesService::getAllStorages()
+	 * @deprecated 8.2.0 use UserStoragesService::getStorages()
 	 */
 	public static function getPersonalMountPoints() {
 		$mountPoints = [];
 		$service = self::$app->getContainer()->query('OCA\Files_External\Service\UserStoragesService');
 
-		foreach ($service->getAllStorages() as $storage) {
+		foreach ($service->getStorages() as $storage) {
 			$mountPoints[] = self::prepareMountPointEntry($storage, true);
 		}
 
diff --git a/apps/files_external/lib/config/configadapter.php b/apps/files_external/lib/config/configadapter.php
index cb8c2f24caa..fb36e011655 100644
--- a/apps/files_external/lib/config/configadapter.php
+++ b/apps/files_external/lib/config/configadapter.php
@@ -133,7 +133,7 @@ class ConfigAdapter implements IMountProvider {
 			$mounts[$storage->getMountPoint()] = $mount;
 		}
 
-		foreach ($this->userStoragesService->getAllStorages() as $storage) {
+		foreach ($this->userStoragesService->getStorages() as $storage) {
 			try {
 				$this->prepareStorageConfig($storage, $user);
 				$impl = $this->constructStorage($storage);
diff --git a/apps/files_external/personal.php b/apps/files_external/personal.php
index efd23512ffe..f048d65540b 100644
--- a/apps/files_external/personal.php
+++ b/apps/files_external/personal.php
@@ -54,7 +54,7 @@ foreach ($authMechanisms as $authMechanism) {
 $tmpl = new OCP\Template('files_external', 'settings');
 $tmpl->assign('encryptionEnabled', \OC::$server->getEncryptionManager()->isEnabled());
 $tmpl->assign('isAdminPage', false);
-$tmpl->assign('storages', $userStoragesService->getAllStorages());
+$tmpl->assign('storages', $userStoragesService->getStorages());
 $tmpl->assign('dependencies', OC_Mount_Config::dependencyMessage($backendService->getBackends()));
 $tmpl->assign('backends', $backends);
 $tmpl->assign('authMechanisms', $authMechanisms);
diff --git a/apps/files_external/service/globalstoragesservice.php b/apps/files_external/service/globalstoragesservice.php
index 0e2d3f2b9c1..49ffea43d1b 100644
--- a/apps/files_external/service/globalstoragesservice.php
+++ b/apps/files_external/service/globalstoragesservice.php
@@ -210,4 +210,13 @@ class GlobalStoragesService extends StoragesService {
 			);
 		}
 	}
+
+	/**
+	 * Get the visibility type for this controller, used in validation
+	 *
+	 * @return string BackendService::VISIBILITY_* constants
+	 */
+	public function getVisibilityType() {
+		return BackendService::VISIBILITY_ADMIN;
+	}
 }
diff --git a/apps/files_external/service/storagesservice.php b/apps/files_external/service/storagesservice.php
index 703f277d84e..83a82de0bed 100644
--- a/apps/files_external/service/storagesservice.php
+++ b/apps/files_external/service/storagesservice.php
@@ -29,6 +29,8 @@ use \OC\Files\Filesystem;
 use \OCA\Files_external\Lib\StorageConfig;
 use \OCA\Files_external\NotFoundException;
 use \OCA\Files_External\Service\BackendService;
+use \OCA\Files_External\Lib\Backend\Backend;
+use \OCA\Files_External\Lib\Auth\AuthMechanism;
 
 /**
  * Service class to manage external storages
@@ -331,7 +333,7 @@ abstract class StoragesService {
 	}
 
 	/**
-	 * Gets all storages
+	 * Gets all storages, valid or not
 	 *
 	 * @return array array of storage configs
 	 */
@@ -340,6 +342,47 @@ abstract class StoragesService {
 	}
 
 	/**
+	 * Gets all valid storages
+	 *
+	 * @return array
+	 */
+	public function getStorages() {
+		return array_filter($this->getAllStorages(), [$this, 'validateStorage']);
+	}
+
+	/**
+	 * Validate storage
+	 * FIXME: De-duplicate with StoragesController::validate()
+	 *
+	 * @param StorageConfig $storage
+	 * @return bool
+	 */
+	protected function validateStorage(StorageConfig $storage) {
+		/** @var Backend */
+		$backend = $storage->getBackend();
+		/** @var AuthMechanism */
+		$authMechanism = $storage->getAuthMechanism();
+
+		if (!$backend->isVisibleFor($this->getVisibilityType())) {
+			// not permitted to use backend
+			return false;
+		}
+		if (!$authMechanism->isVisibleFor($this->getVisibilityType())) {
+			// not permitted to use auth mechanism
+			return false;
+		}
+
+		return true;
+	}
+
+	/**
+	 * Get the visibility type for this controller, used in validation
+	 *
+	 * @return string BackendService::VISIBILITY_* constants
+	 */
+	abstract public function getVisibilityType();
+
+	/**
 	 * Add new storage to the configuration
 	 *
 	 * @param array $newStorage storage attributes
diff --git a/apps/files_external/service/userglobalstoragesservice.php b/apps/files_external/service/userglobalstoragesservice.php
index b60473f131e..7c60bc6d497 100644
--- a/apps/files_external/service/userglobalstoragesservice.php
+++ b/apps/files_external/service/userglobalstoragesservice.php
@@ -117,7 +117,7 @@ class UserGlobalStoragesService extends GlobalStoragesService {
 	 * @return StorageConfig[]
 	 */
 	public function getUniqueStorages() {
-		$storages = $this->getAllStorages();
+		$storages = $this->getStorages();
 
 		$storagesByMountpoint = [];
 		foreach ($storages as $storage) {
diff --git a/apps/files_external/service/userstoragesservice.php b/apps/files_external/service/userstoragesservice.php
index c69b8d4f51e..6e3c327c09c 100644
--- a/apps/files_external/service/userstoragesservice.php
+++ b/apps/files_external/service/userstoragesservice.php
@@ -171,4 +171,13 @@ class UserStoragesService extends StoragesService {
 			$this->triggerHooks($newStorage, Filesystem::signal_create_mount);
 		}
 	}
+
+	/**
+	 * Get the visibility type for this controller, used in validation
+	 *
+	 * @return string BackendService::VISIBILITY_* constants
+	 */
+	public function getVisibilityType() {
+		return BackendService::VISIBILITY_PERSONAL;
+	}
 }
diff --git a/apps/files_external/settings.php b/apps/files_external/settings.php
index 5c920a1495b..f82ab035c1c 100644
--- a/apps/files_external/settings.php
+++ b/apps/files_external/settings.php
@@ -65,7 +65,7 @@ $userBackends = array_filter($backendService->getAvailableBackends(), function($
 $tmpl = new OCP\Template('files_external', 'settings');
 $tmpl->assign('encryptionEnabled', \OC::$server->getEncryptionManager()->isEnabled());
 $tmpl->assign('isAdminPage', true);
-$tmpl->assign('storages', $globalStoragesService->getAllStorages());
+$tmpl->assign('storages', $globalStoragesService->getStorages());
 $tmpl->assign('backends', $backends);
 $tmpl->assign('authMechanisms', $authMechanisms);
 $tmpl->assign('userBackends', $userBackends);
diff --git a/apps/files_external/tests/controller/globalstoragescontrollertest.php b/apps/files_external/tests/controller/globalstoragescontrollertest.php
index e1bfad8caf6..6b020198bd8 100644
--- a/apps/files_external/tests/controller/globalstoragescontrollertest.php
+++ b/apps/files_external/tests/controller/globalstoragescontrollertest.php
@@ -24,6 +24,7 @@ use \OCA\Files_external\Controller\GlobalStoragesController;
 use \OCA\Files_external\Service\GlobalStoragesService;
 use \OCP\AppFramework\Http;
 use \OCA\Files_external\NotFoundException;
+use \OCA\Files_External\Service\BackendService;
 
 class GlobalStoragesControllerTest extends StoragesControllerTest {
 	public function setUp() {
@@ -32,6 +33,9 @@ class GlobalStoragesControllerTest extends StoragesControllerTest {
 			->disableOriginalConstructor()
 			->getMock();
 
+		$this->service->method('getVisibilityType')
+			->willReturn(BackendService::VISIBILITY_ADMIN);
+
 		$this->controller = new GlobalStoragesController(
 			'files_external',
 			$this->getMock('\OCP\IRequest'),
diff --git a/apps/files_external/tests/controller/userstoragescontrollertest.php b/apps/files_external/tests/controller/userstoragescontrollertest.php
index 9f1a8df8d2e..33ef274c87c 100644
--- a/apps/files_external/tests/controller/userstoragescontrollertest.php
+++ b/apps/files_external/tests/controller/userstoragescontrollertest.php
@@ -40,6 +40,9 @@ class UserStoragesControllerTest extends StoragesControllerTest {
 			->disableOriginalConstructor()
 			->getMock();
 
+		$this->service->method('getVisibilityType')
+			->willReturn(BackendService::VISIBILITY_PERSONAL);
+
 		$this->controller = new UserStoragesController(
 			'files_external',
 			$this->getMock('\OCP\IRequest'),
diff --git a/apps/files_external/tests/service/storagesservicetest.php b/apps/files_external/tests/service/storagesservicetest.php
index 28220c9bc2e..ddf52e6272e 100644
--- a/apps/files_external/tests/service/storagesservicetest.php
+++ b/apps/files_external/tests/service/storagesservicetest.php
@@ -305,6 +305,52 @@ abstract class StoragesServiceTest extends \Test\TestCase {
 		);
 	}
 
+	public function testGetStoragesBackendNotVisible() {
+		$backend = $this->backendService->getBackend('identifier:\OCA\Files_External\Lib\Backend\SMB');
+		$backend->expects($this->once())
+			->method('isVisibleFor')
+			->with($this->service->getVisibilityType())
+			->willReturn(false);
+		$authMechanism = $this->backendService->getAuthMechanism('identifier:\Auth\Mechanism');
+		$authMechanism->method('isVisibleFor')
+			->with($this->service->getVisibilityType())
+			->willReturn(true);
+
+		$storage = new StorageConfig(255);
+		$storage->setMountPoint('mountpoint');
+		$storage->setBackend($backend);
+		$storage->setAuthMechanism($authMechanism);
+		$storage->setBackendOptions(['password' => 'testPassword']);
+
+		$newStorage = $this->service->addStorage($storage);
+
+		$this->assertCount(1, $this->service->getAllStorages());
+		$this->assertEmpty($this->service->getStorages());
+	}
+
+	public function testGetStoragesAuthMechanismNotVisible() {
+		$backend = $this->backendService->getBackend('identifier:\OCA\Files_External\Lib\Backend\SMB');
+		$backend->method('isVisibleFor')
+			->with($this->service->getVisibilityType())
+			->willReturn(true);
+		$authMechanism = $this->backendService->getAuthMechanism('identifier:\Auth\Mechanism');
+		$authMechanism->expects($this->once())
+			->method('isVisibleFor')
+			->with($this->service->getVisibilityType())
+			->willReturn(false);
+
+		$storage = new StorageConfig(255);
+		$storage->setMountPoint('mountpoint');
+		$storage->setBackend($backend);
+		$storage->setAuthMechanism($authMechanism);
+		$storage->setBackendOptions(['password' => 'testPassword']);
+
+		$newStorage = $this->service->addStorage($storage);
+
+		$this->assertCount(1, $this->service->getAllStorages());
+		$this->assertEmpty($this->service->getStorages());
+	}
+
 	public static function createHookCallback($params) {
 		self::$hookCalls[] = array(
 			'signal' => Filesystem::signal_create_mount,
diff --git a/apps/files_external/tests/service/userglobalstoragesservicetest.php b/apps/files_external/tests/service/userglobalstoragesservicetest.php
index 867872f3683..b6dc952605d 100644
--- a/apps/files_external/tests/service/userglobalstoragesservicetest.php
+++ b/apps/files_external/tests/service/userglobalstoragesservicetest.php
@@ -209,7 +209,11 @@ class UserGlobalStoragesServiceTest extends GlobalStoragesServiceTest {
 		$expectedPrecedence
 	) {
 		$backend = $this->backendService->getBackend('identifier:\OCA\Files_External\Lib\Backend\SMB');
+		$backend->method('isVisibleFor')
+			->willReturn(true);
 		$authMechanism = $this->backendService->getAuthMechanism('identifier:\Auth\Mechanism');
+		$authMechanism->method('isVisibleFor')
+			->willReturn(true);
 
 		$storage1 = new StorageConfig();
 		$storage1->setMountPoint('mountpoint');
@@ -243,6 +247,16 @@ class UserGlobalStoragesServiceTest extends GlobalStoragesServiceTest {
 		}
 	}
 
+	public function testGetStoragesBackendNotVisible() {
+		// we don't test this here
+		$this->assertTrue(true);
+	}
+
+	public function testGetStoragesAuthMechanismNotVisible() {
+		// we don't test this here
+		$this->assertTrue(true);
+	}
+
 	public function testHooksAddStorage($a = null, $b = null, $c = null) {
 		// we don't test this here
 		$this->assertTrue(true);